| Message ID | 1701246516-11626-1-git-send-email-quic_cang@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable HS-G5 support on SM8550 | expand |
On 11/29/2023 1:58 PM, Can Guo wrote: > In ufs_qcom_power_up_sequence(), check return value of phy_set_mode_ext() > and stop proceeding if phy_set_mode_ext() fails. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 30f4ca6..9c0ebbc 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -475,7 +475,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > return ret; > } > > - phy_set_mode_ext(phy, mode, host->phy_gear); > + ret = phy_set_mode_ext(phy, mode, host->phy_gear); > + if (ret) { > + dev_err(hba->dev, "%s: phy set mode failed, ret = %d\n", > + __func__, ret); > + goto out_disable_phy; > + } > > /* power on phy - start serdes and phy's power and clocks */ > ret = phy_power_on(phy); Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
On 11/29/23 00:28, Can Guo wrote: > Structure ufs_dev_params is actually used in UFS host vendor drivers to > declare host specific power mode parameters, like ufs_<vendor>_params or > host_cap, which makes the code not very straightforward to read. Rename the > structure ufs_dev_params to ufs_host_params and unify the declarations in > all vendor drivers to host_params. > > In addition, rename the two functions ufshcd_init_pwr_dev_param() and > ufshcd_get_pwr_dev_param() which work based on the ufs_host_params to > ufshcd_init_host_params() and ufshcd_negotiate_pwr_params() respectively to > avoid confusions. > > This change does not change any functionalities or logic. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Wed, Nov 29, 2023 at 12:28:34AM -0800, Can Guo wrote: > In ufs_qcom_power_up_sequence(), check return value of phy_set_mode_ext() > and stop proceeding if phy_set_mode_ext() fails. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 30f4ca6..9c0ebbc 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -475,7 +475,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > return ret; > } > > - phy_set_mode_ext(phy, mode, host->phy_gear); > + ret = phy_set_mode_ext(phy, mode, host->phy_gear); > + if (ret) { > + dev_err(hba->dev, "%s: phy set mode failed, ret = %d\n", > + __func__, ret); No need to print the error message here as it is already done in the PHY driver. Also, this patch should come before the PHY patch returning error. - Mani > + goto out_disable_phy; > + } > > /* power on phy - start serdes and phy's power and clocks */ > ret = phy_power_on(phy); > -- > 2.7.4 > >
On Wed, Nov 29, 2023 at 12:28:35AM -0800, Can Guo wrote: > From: "Bao D. Nguyen" <quic_nguyenb@quicinc.com> > > A spare register in UFS host controller is used to indicate the UFS device > version. The spare register is populated by bootloader for now, but in > future it will be populated by HW automatically during link startup with > its best efforts in any boot stages prior to Linux. > > During host driver init, read the spare register, if it is not populated > with a UFS device version, go ahead with the dual init mechanism. If a UFS > device version is in there, use the UFS device version together with host > controller's HW version to decide the proper PHY gear which should be used > to configure the UFS PHY without going through the second init. > > Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 23 ++++++++++++++++++----- > drivers/ufs/host/ufs-qcom.h | 2 ++ > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 9c0ebbc..e94dea2 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1068,15 +1068,28 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) > { > struct ufs_host_params *host_params = &host->host_params; > + u32 val, dev_major = 0; No need to initialize dev_major. > > host->phy_gear = host_params->hs_tx_gear; > > - /* > - * Power up the PHY using the minimum supported gear (UFS_HS_G2). > - * Switching to max gear will be performed during reinit if supported. > - */ > - if (host->hw_ver.major < 0x4) > + if (host->hw_ver.major < 0x4) { > + /* > + * Power up the PHY using the minimum supported gear (UFS_HS_G2). > + * Switching to max gear will be performed during reinit if supported. > + */ > host->phy_gear = UFS_HS_G2; > + } else { Can you please add a comment here to describe what is happening? > + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); > + dev_major = FIELD_GET(GENMASK(7, 4), val); It'd be good to add a macro for GENMASK(). > + > + /* UFS device version populated, no need to do init twice */ "Since the UFS device version is populated, let's remove the REINIT quirk as the negotiated gear won't change during boot. So there is no need to do reinit." > + if (dev_major != 0) 0x0 > + host->hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > + > + /* For UFS 3.1 and older, apply HS-G4 PHY gear to save power */ > + if (dev_major < 0x4 && dev_major > 0) if (dev_major > 0x0 && dev_major < 0x4) - Mani > + host->phy_gear = UFS_HS_G4; > + } > } > > static void ufs_qcom_set_host_params(struct ufs_hba *hba) > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 11419eb..d12fc5a 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -54,6 +54,8 @@ enum { > UFS_AH8_CFG = 0xFC, > > REG_UFS_CFG3 = 0x271C, > + > + REG_UFS_DEBUG_SPARE_CFG = 0x284C, > }; > > /* QCOM UFS host controller vendor specific debug registers */ > -- > 2.7.4 > >
On 11/30/2023 3:16 PM, Manivannan Sadhasivam wrote: > On Wed, Nov 29, 2023 at 12:28:34AM -0800, Can Guo wrote: >> In ufs_qcom_power_up_sequence(), check return value of phy_set_mode_ext() >> and stop proceeding if phy_set_mode_ext() fails. >> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 30f4ca6..9c0ebbc 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -475,7 +475,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) >> return ret; >> } >> >> - phy_set_mode_ext(phy, mode, host->phy_gear); >> + ret = phy_set_mode_ext(phy, mode, host->phy_gear); >> + if (ret) { >> + dev_err(hba->dev, "%s: phy set mode failed, ret = %d\n", >> + __func__, ret); > > No need to print the error message here as it is already done in the PHY driver. > > Also, this patch should come before the PHY patch returning error. Sure. Thanks, Can Guo. > > - Mani > >> + goto out_disable_phy; >> + } >> >> /* power on phy - start serdes and phy's power and clocks */ >> ret = phy_power_on(phy); >> -- >> 2.7.4 >> >> >
On 11/30/2023 3:25 PM, Manivannan Sadhasivam wrote: > On Wed, Nov 29, 2023 at 12:28:35AM -0800, Can Guo wrote: >> From: "Bao D. Nguyen" <quic_nguyenb@quicinc.com> >> >> A spare register in UFS host controller is used to indicate the UFS device >> version. The spare register is populated by bootloader for now, but in >> future it will be populated by HW automatically during link startup with >> its best efforts in any boot stages prior to Linux. >> >> During host driver init, read the spare register, if it is not populated >> with a UFS device version, go ahead with the dual init mechanism. If a UFS >> device version is in there, use the UFS device version together with host >> controller's HW version to decide the proper PHY gear which should be used >> to configure the UFS PHY without going through the second init. >> >> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 23 ++++++++++++++++++----- >> drivers/ufs/host/ufs-qcom.h | 2 ++ >> 2 files changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 9c0ebbc..e94dea2 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -1068,15 +1068,28 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) >> static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) >> { >> struct ufs_host_params *host_params = &host->host_params; >> + u32 val, dev_major = 0; > > No need to initialize dev_major. True. > >> >> host->phy_gear = host_params->hs_tx_gear; >> >> - /* >> - * Power up the PHY using the minimum supported gear (UFS_HS_G2). >> - * Switching to max gear will be performed during reinit if supported. >> - */ >> - if (host->hw_ver.major < 0x4) >> + if (host->hw_ver.major < 0x4) { >> + /* >> + * Power up the PHY using the minimum supported gear (UFS_HS_G2). >> + * Switching to max gear will be performed during reinit if supported. >> + */ >> host->phy_gear = UFS_HS_G2; >> + } else { > > Can you please add a comment here to describe what is happening? Will do. > >> + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); >> + dev_major = FIELD_GET(GENMASK(7, 4), val); > > It'd be good to add a macro for GENMASK(). OK > >> + >> + /* UFS device version populated, no need to do init twice */ > > "Since the UFS device version is populated, let's remove the REINIT quirk as the > negotiated gear won't change during boot. So there is no need to do reinit." > Nice comment. >> + if (dev_major != 0) > > 0x0 Done > >> + host->hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; >> + >> + /* For UFS 3.1 and older, apply HS-G4 PHY gear to save power */ >> + if (dev_major < 0x4 && dev_major > 0) > > if (dev_major > 0x0 && dev_major < 0x4) > Sure. Thanks, Can Guo. > - Mani > >> + host->phy_gear = UFS_HS_G4; >> + } >> } >> >> static void ufs_qcom_set_host_params(struct ufs_hba *hba) >> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h >> index 11419eb..d12fc5a 100644 >> --- a/drivers/ufs/host/ufs-qcom.h >> +++ b/drivers/ufs/host/ufs-qcom.h >> @@ -54,6 +54,8 @@ enum { >> UFS_AH8_CFG = 0xFC, >> >> REG_UFS_CFG3 = 0x271C, >> + >> + REG_UFS_DEBUG_SPARE_CFG = 0x284C, >> }; >> >> /* QCOM UFS host controller vendor specific debug registers */ >> -- >> 2.7.4 >> >> >
This series enables HS-G5 support on SM8550. This series is rebased on below changes from Mani - https://patchwork.kernel.org/project/linux-scsi/patch/20230908145329.154024-1-manivannan.sadhasivam@linaro.org/ https://patchwork.kernel.org/project/linux-scsi/patch/20230908145329.154024-2-manivannan.sadhasivam@linaro.org/ This series is tested on below HW combinations - SM8550 MTP + UFS4.0 SM8550 QRD + UFS3.1 SM8450 MTP + UFS3.1 (for regression test) SM8350 MTP + UFS3.1 (for regression test) Note that during reboot test on above platforms, I occasinally hit PA (PHY) error during the 2nd init, this is not related with this series. A fix for this is mentioned in below patchwork - https://patchwork.kernel.org/project/linux-scsi/patch/1698145815-17396-1-git-send-email-quic_ziqichen@quicinc.com/ Also note that on platforms, which have two sets of UFS PHY settings are provided (say G4 and no-G4, G5 and no-G5). The two sets of PHY settings are basically 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. For SM8550, this series will make sure we honor the rule. However, for old targets Mani and I will fix them in another series in future. v5 -> v6: 1. Rebased on scsi-queue-6.8 2. Addressed comments from Dmitry and Mani in patches to phy-qcom-qmp-ufs.c v4 -> v5: Removed two useless debug prints in patch #9 v3 -> v4: Used .tbls_hs_overlay array instead of adding more tables with different names like .tbls_hs_g5 v2 -> v3: 1. Addressed comments from Andrew, Mani and Bart in patch #1 2. Added patch #2 as per request from Andrew and Mani 3. Added patch #4 to fix a common issue on old targets, it is not necessary for this series, but put in this series only because it would be easier to maintain and no need to rebase 4. Addressed comments from Dmitry and Mani in patches to phy-qcom-qmp-ufs.c v1 -> v2: 1. Removed 2 changes which were exposing power info in sysfs 2. Removed 1 change which was moving data structs to phy-qcom-qmp-ufs.h 3. Added one new change (the 1st one) to clean up usage of ufs_dev_params based on comments from Mani 4. Adjusted the logic of UFS device version detection according to comments from Mani: 4.1 For HW version < 0x5, go through dual init 4.2 For HW version >= 0x5 a. If UFS device version is populated, one init is required b. If UFS device version is not populated, go through dual init Bao D. Nguyen (1): scsi: ufs: ufs-qcom: Add support for UFS device version detection Can Guo (9): scsi: ufs: host: Rename structure ufs_dev_params to ufs_host_params scsi: ufs: ufs-qcom: No need to set hs_rate after ufshcd_init_host_param() scsi: ufs: ufs-qcom: Setup host power mode during init scsi: ufs: ufs-qcom: Allow the first init start with the maximum supported gear scsi: ufs: ufs-qcom: Limit HS-G5 Rate-A to hosts with HW version 5 scsi: ufs: ufs-qcom: Set initial PHY gear to max HS gear for HW ver 4 and newer phy: qualcomm: phy-qcom-qmp-ufs: Rectify SM8550 UFS HS-G4 PHY Settings phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed Gear 5 support for SM8550 scsi: ufs: ufs-qcom: Check return value of phy_set_mode_ext() 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 | 192 ++++++++++++++++++--- drivers/ufs/host/ufs-exynos.c | 7 +- drivers/ufs/host/ufs-hisi.c | 11 +- drivers/ufs/host/ufs-mediatek.c | 12 +- drivers/ufs/host/ufs-qcom.c | 90 +++++++--- drivers/ufs/host/ufs-qcom.h | 5 +- drivers/ufs/host/ufshcd-pltfrm.c | 69 ++++---- drivers/ufs/host/ufshcd-pltfrm.h | 10 +- 11 files changed, 306 insertions(+), 106 deletions(-)