[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e8fce711-dfe3-612c-dc53-f45418c9dfeb@quicinc.com>
Date: Wed, 8 Nov 2023 17:19:43 +0800
From: Can Guo <quic_cang@...cinc.com>
To: Manivannan Sadhasivam <mani@...nel.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Can Guo <cang@....qualcomm.com>, <bvanassche@....org>,
<stanley.chu@...iatek.com>, <adrian.hunter@...el.com>,
<beanhuo@...ron.com>, <avri.altman@....com>,
<junwoo80.lee@...sung.com>, <martin.petersen@...cle.com>,
<linux-scsi@...r.kernel.org>, Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Vinod Koul <vkoul@...nel.org>,
"Kishon Vijay Abraham I" <kishon@...nel.org>,
"open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
"open list:GENERIC PHY FRAMEWORK" <linux-phy@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 6/7] phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed
Gear 5 support for SM8550
Hi Mani,
On 11/8/2023 1:49 PM, Manivannan Sadhasivam wrote:
> On Tue, Nov 07, 2023 at 03:18:09PM +0200, Dmitry Baryshkov wrote:
>> On Tue, 7 Nov 2023 at 06:47, Can Guo <cang@....qualcomm.com> wrote:
>>>
>>> From: Can Guo <quic_cang@...cinc.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.
>>>
>
> You should also mention that this issue is also present in G4 supported targets.
> And a note that it will get fixed later.
Will this info upset more people? How about I mention this in the cover
letter?
>
>>> Signed-off-by: Can Guo <quic_cang@...cinc.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.
>
> Ack
>
>>
>>> +
>>> + 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?
>>
>
> We use G4 table for all the modes <= G4.
>
>>> +};
>>> +
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g5_serdes[] = {
>>> + QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_IVCO, 0x1f),
>>> +
>>> + QMP_PHY_INIT_CFG(QSERDES_V6_COM_CMN_IETRIM, 0x1b),
>>> + QMP_PHY_INIT_CFG(QSERDES_V6_COM_CMN_IPTRIM, 0x1c),
>>> + QMP_PHY_INIT_CFG(QSERDES_V6_COM_CP_CTRL_MODE0, 0x06),
>>> };
>>>
>>> static const struct qmp_phy_init_tbl sm8550_ufsphy_tx[] = {
>>> - QMP_PHY_INIT_CFG(QSERDES_V6_TX_LANE_MODE_1, 0x05),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_TX_LANE_MODE_1, 0x05),
>>> QMP_PHY_INIT_CFG(QSERDES_UFS_V6_TX_RES_CODE_LANE_OFFSET_TX, 0x07),
>>> };
>>>
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g4_tx[] = {
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_TX_FR_DCC_CTRL, 0x4c),
>>> +};
>>> +
>>> static const struct qmp_phy_init_tbl sm8550_ufsphy_rx[] = {
>>> - QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FASTLOCK_FO_GAIN_RATE2, 0x0c),
>>> - QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FASTLOCK_FO_GAIN_RATE4, 0x0f),
>>> - QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_VGA_CAL_MAN_VAL, 0x0e),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FO_GAIN_RATE2, 0x0c),
>>>
>>> QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE_0_1_B0, 0xc2),
>>> QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE_0_1_B1, 0xc2),
>>> @@ -690,14 +709,46 @@ static const struct qmp_phy_init_tbl sm8550_ufsphy_rx[] = {
>>> QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE3_B8, 0x02),
>>> };
>>>
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g4_rx[] = {
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_VGA_CAL_MAN_VAL, 0x0e),
>>> +};
>>> +
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g5_rx[] = {
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FO_GAIN_RATE4, 0x0c),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_SO_GAIN_RATE4, 0x04),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x14),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_PI_CONTROLS, 0x07),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_OFFSET_ADAPTOR_CNTRL3, 0x0e),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FASTLOCK_COUNT_HIGH_RATE4, 0x02),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FASTLOCK_FO_GAIN_RATE4, 0x1c),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_UCDR_FASTLOCK_SO_GAIN_RATE4, 0x06),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_VGA_CAL_MAN_VAL, 0x08),
>>> +
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE4_B3, 0xb9),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE4_B4, 0x4f),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_MODE_RATE4_B6, 0xff),
>>> + QMP_PHY_INIT_CFG(QSERDES_UFS_V6_RX_DLL0_FTUNE_CTRL, 0x30),
>>> +};
>>> +
>>> static const struct qmp_phy_init_tbl sm8550_ufsphy_pcs[] = {
>>> QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_RX_SIGDET_CTRL2, 0x69),
>>> QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0f),
>>> QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_TX_MID_TERM_CTRL1, 0x43),
>>> - QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_PLL_CNTL, 0x2b),
>>> QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
>>> };
>>>
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g4_pcs[] = {
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_PLL_CNTL, 0x2b),
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_TX_HSGEAR_CAPABILITY, 0x04),
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_RX_HSGEAR_CAPABILITY, 0x04),
>>> +};
>>> +
>>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g5_pcs[] = {
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_PLL_CNTL, 0x33),
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_RX_HS_G5_SYNC_LENGTH_CAPABILITY, 0x4f),
>>> + QMP_PHY_INIT_CFG(QPHY_V6_PCS_UFS_RX_HSG5_SYNC_WAIT_TIME, 0x9e),
>>> +};
>>> +
>>> struct qmp_ufs_offsets {
>>> u16 serdes;
>>> u16 pcs;
>>> @@ -731,6 +782,8 @@ struct qmp_phy_cfg {
>>> const struct qmp_phy_cfg_tbls tbls_hs_b;
>>> /* Additional sequence for HS G4 */
>>> const struct qmp_phy_cfg_tbls tbls_hs_g4;
>>> + /* Additional sequence for HS G4 */
>
> HS G5
Sure
>
>>> + const struct qmp_phy_cfg_tbls tbls_hs_g5;
>>>
>>> /* clock ids to be requested */
>>> const char * const *clk_list;
>>> @@ -1157,6 +1210,28 @@ static const struct qmp_phy_cfg sm8550_ufsphy_cfg = {
>>> .pcs = sm8550_ufsphy_pcs,
>>> .pcs_num = ARRAY_SIZE(sm8550_ufsphy_pcs),
>>> },
>>> + .tbls_hs_b = {
>>> + .serdes = sm8550_ufsphy_hs_b_serdes,
>>> + .serdes_num = ARRAY_SIZE(sm8550_ufsphy_hs_b_serdes),
>>> + },
>>> + .tbls_hs_g4 = {
>>> + .serdes = sm8550_ufsphy_g4_serdes,
>>> + .serdes_num = ARRAY_SIZE(sm8550_ufsphy_g4_serdes),
>>> + .tx = sm8550_ufsphy_g4_tx,
>>> + .tx_num = ARRAY_SIZE(sm8550_ufsphy_g4_tx),
>>> + .rx = sm8550_ufsphy_g4_rx,
>>> + .rx_num = ARRAY_SIZE(sm8550_ufsphy_g4_rx),
>>> + .pcs = sm8550_ufsphy_g4_pcs,
>>> + .pcs_num = ARRAY_SIZE(sm8550_ufsphy_g4_pcs),
>>> + },
>>> + .tbls_hs_g5 = {
>>> + .serdes = sm8550_ufsphy_g5_serdes,
>>> + .serdes_num = ARRAY_SIZE(sm8550_ufsphy_g5_serdes),
>>> + .rx = sm8550_ufsphy_g5_rx,
>>> + .rx_num = ARRAY_SIZE(sm8550_ufsphy_g5_rx),
>>> + .pcs = sm8550_ufsphy_g5_pcs,
>>> + .pcs_num = ARRAY_SIZE(sm8550_ufsphy_g5_pcs),
>>> + },
>>> .clk_list = sdm845_ufs_phy_clk_l,
>>> .num_clks = ARRAY_SIZE(sdm845_ufs_phy_clk_l),
>>> .vreg_list = qmp_phy_vreg_l,
>>> @@ -1222,14 +1297,25 @@ static void qmp_ufs_pcs_init(struct qmp_ufs *qmp, const struct qmp_phy_cfg_tbls
>>> static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg *cfg)
>>> {
>>> qmp_ufs_serdes_init(qmp, &cfg->tbls);
>>> + if (qmp->submode == UFS_HS_G4)
>>> + qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_g4);
>>> + else if (qmp->submode == UFS_HS_G5)
>>> + qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_g5);
>>> +
>
> Should we program submode sequence after HS_B?
>
> - Mani
>
As the UFS PHY HW programming doc mentions, order of these writes is not
important, so long as they are programmed correctly before release S/W
reset, then it is fine.
Thanks,
Can Guo.
Powered by blists - more mailing lists