| Message ID | 1700729190-17268-1-git-send-email-quic_cang@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable HS-G5 support on SM8550 | expand |
On 11/23/2023 2:16 PM, Can Guo wrote: > In ufs_qcom_pwr_change_notify(), host_params.hs_rate has been set to > PA_HS_MODE_B by ufshcd_init_host_param(), hence remove the duplicated line > of work. Meanwhile, removed the macro UFS_QCOM_LIMIT_HS_RATE as it is only > used here. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 1 - > drivers/ufs/host/ufs-qcom.h | 2 -- > 2 files changed, 3 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index aee66a3..cc30ad9 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -909,7 +909,6 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > switch (status) { > case PRE_CHANGE: > ufshcd_init_host_param(&host_params); > - host_params.hs_rate = UFS_QCOM_LIMIT_HS_RATE; > > /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */ > host_params.hs_tx_gear = host_params.hs_rx_gear = ufs_qcom_get_hs_gear(hba); > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 9950a00..82cd143 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -27,8 +27,6 @@ > #define SLOW 1 > #define FAST 2 > > -#define UFS_QCOM_LIMIT_HS_RATE PA_HS_MODE_B > - > /* QCOM UFS host controller vendor specific registers */ > enum { > REG_UFS_SYS1CLK_1US = 0xC0, Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
On Thu, Nov 23, 2023 at 12:46:22AM -0800, Can Guo wrote: > In ufs_qcom_pwr_change_notify(), host_params.hs_rate has been set to > PA_HS_MODE_B by ufshcd_init_host_param(), hence remove the duplicated line > of work. Meanwhile, removed the macro UFS_QCOM_LIMIT_HS_RATE as it is only > used here. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/host/ufs-qcom.c | 1 - > drivers/ufs/host/ufs-qcom.h | 2 -- > 2 files changed, 3 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index aee66a3..cc30ad9 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -909,7 +909,6 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > switch (status) { > case PRE_CHANGE: > ufshcd_init_host_param(&host_params); > - host_params.hs_rate = UFS_QCOM_LIMIT_HS_RATE; > > /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */ > host_params.hs_tx_gear = host_params.hs_rx_gear = ufs_qcom_get_hs_gear(hba); > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 9950a00..82cd143 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -27,8 +27,6 @@ > #define SLOW 1 > #define FAST 2 > > -#define UFS_QCOM_LIMIT_HS_RATE PA_HS_MODE_B > - > /* QCOM UFS host controller vendor specific registers */ > enum { > REG_UFS_SYS1CLK_1US = 0xC0, > -- > 2.7.4 >
On Tue, Nov 28, 2023 at 10:49:36AM +0530, Manivannan Sadhasivam wrote: > On Thu, Nov 23, 2023 at 12:46:21AM -0800, 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_param() and ufshcd_negotiate_pwr_param() respectively to > > avoid confusions. > > > > This change does not change any functionalities or logic. > > > > Acked-by: Andrew Halaney <ahalaney@redhat.com> > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > - Mani > > > --- > > 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 | 12 +++---- > > drivers/ufs/host/ufshcd-pltfrm.c | 69 ++++++++++++++++++++-------------------- > > drivers/ufs/host/ufshcd-pltfrm.h | 10 +++--- > > 6 files changed, 57 insertions(+), 64 deletions(-) > > [...] > > diff --git a/drivers/ufs/host/ufshcd-pltfrm.h b/drivers/ufs/host/ufshcd-pltfrm.h > > index a86a3ad..2d4d047 100644 > > --- a/drivers/ufs/host/ufshcd-pltfrm.h > > +++ b/drivers/ufs/host/ufshcd-pltfrm.h > > @@ -10,7 +10,7 @@ > > #define UFS_PWM_MODE 1 > > #define UFS_HS_MODE 2 > > > > -struct ufs_dev_params { > > +struct ufs_host_params { > > u32 pwm_rx_gear; /* pwm rx gear to work in */ > > u32 pwm_tx_gear; /* pwm tx gear to work in */ > > u32 hs_rx_gear; /* hs rx gear to work in */ > > @@ -25,10 +25,10 @@ struct ufs_dev_params { > > u32 desired_working_mode; > > }; > > > > -int ufshcd_get_pwr_dev_param(const struct ufs_dev_params *dev_param, > > - const struct ufs_pa_layer_attr *dev_max, > > - struct ufs_pa_layer_attr *agreed_pwr); > > -void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param); > > +int ufshcd_negotiate_pwr_param(const struct ufs_host_params *host_param, > > + const struct ufs_pa_layer_attr *dev_max, > > + struct ufs_pa_layer_attr *agreed_pwr); > > +void ufshcd_init_host_param(struct ufs_host_params *host_param); Noticed this after giving my R-b tag. Could you please rename the functions to: int ufshcd_negotiate_pwr_params(const struct ufs_host_params *host_params,... void ufshcd_init_host_params(struct ufs_host_params *host_params); Not a big deal, but since the argument passed to both functions are 'params', it'd be good if the function definition also use the same plural form. - Mani
On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote: > In the dual init scenario, the initial PHY gear is set to HS-G2, and the > first Power Mode Change (PMC) is meant to find the best matching PHY gear > for the 2nd init. However, for the first PMC, if the negotiated gear (say > HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to > the negotiated gear happen, because the programmed UFS PHY settings may not > support the negotiated gear. Fix it by overwriting the negotiated gear with > the PHY gear. > I don't quite understand this patch. If the phy_gear is G2 initially and the negotiated gear is G4, then as per this change, phy_gear = G4; negotiated gear = G2; Could you please explain how this make sense? - Mani > 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 cc0eb37..d4edf58 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > * because, the PHY gear settings are backwards compatible and we only need to > * change the PHY gear settings while scaling to higher gears. > */ > - if (dev_req_params->gear_tx > host->phy_gear) > + if (dev_req_params->gear_tx > host->phy_gear) { > + u32 old_phy_gear = host->phy_gear; > + > host->phy_gear = dev_req_params->gear_tx; > + dev_req_params->gear_tx = old_phy_gear; > + dev_req_params->gear_rx = old_phy_gear; > + } > > /* enable the device ref clock before changing to HS mode */ > if (!ufshcd_is_hs_mode(&hba->pwr_info) && > -- > 2.7.4 >
On Thu, Nov 23, 2023 at 12:46:27AM -0800, Can Guo wrote: > Set the initial PHY gear to max HS gear for hosts with HW ver 5 and newer. > MAX_GEAR will be used for hosts with hw_ver.major >= 4 > This patch is not changing any functionalities or logic but only a > preparation patch for the next patch in this series. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 6756f8d..7bbccf4 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1067,6 +1067,20 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > } > > +static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) > +{ > + struct ufs_host_params *host_params = &host->host_params; > + > + 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. You need to reword this comment too. > + */ > + if (host->hw_ver.major < 0x5) As I mentioned above, MAX_GEAR will be used if hw_ver.major is >=4 in ufs_qcom_get_hs_gear(). So this check should be (< 0x4). - Mani > + host->phy_gear = UFS_HS_G2; > +} > + > static void ufs_qcom_set_host_params(struct ufs_hba *hba) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > @@ -1303,6 +1317,7 @@ static int ufs_qcom_init(struct ufs_hba *hba) > ufs_qcom_set_caps(hba); > ufs_qcom_advertise_quirks(hba); > ufs_qcom_set_host_params(hba); > + ufs_qcom_set_phy_gear(host); > > err = ufs_qcom_ice_init(host); > if (err) > @@ -1320,12 +1335,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) > dev_warn(dev, "%s: failed to configure the testbus %d\n", > __func__, err); > > - /* > - * 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; > - > return 0; > > out_variant_clear: > -- > 2.7.4 >
Hi Mani, On 11/28/2023 1:55 PM, Manivannan Sadhasivam wrote: > On Thu, Nov 23, 2023 at 12:46:26AM -0800, Can Guo wrote: >> Qcom UFS hosts, with HW ver 5, can only support up to HS-G5 Rate-A due to >> HW limitations. If the HS-G5 PHY gear is used, update host_params->hs_rate >> to Rate-A, so that the subsequent power mode changes shall stick to Rate-A. >> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > One question below... > >> --- >> drivers/ufs/host/ufs-qcom.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 9613ad9..6756f8d 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -442,9 +442,25 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba) >> static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> + struct ufs_host_params *host_params = &host->host_params; >> struct phy *phy = host->generic_phy; >> + enum phy_mode mode; >> int ret; >> >> + /* >> + * HW ver 5 can only support up to HS-G5 Rate-A due to HW limitations. >> + * If the HS-G5 PHY gear is used, update host_params->hs_rate to Rate-A, >> + * so that the subsequent power mode change shall stick to Rate-A. >> + */ >> + if (host->hw_ver.major == 0x5) { >> + if (host->phy_gear == UFS_HS_G5) >> + host_params->hs_rate = PA_HS_MODE_A; >> + else >> + host_params->hs_rate = PA_HS_MODE_B; > > Is this 'else' part really needed? Since there wouldn't be any 2nd init, I think > we can skip that. We need it because, even there is only one init, if a UFS3.1 device is attached, phy_gear is given as UFS_HS_G4 in ufs_qcom_set_phy_gear(), hence we need to put the UFS at HS-G4 Rate B, not Rate A. Thanks, Can Guo. > > - Mani > >> + } >> + >> + mode = host_params->hs_rate == PA_HS_MODE_B ? PHY_MODE_UFS_HS_B : PHY_MODE_UFS_HS_A; >> + >> /* Reset UFS Host Controller and PHY */ >> ret = ufs_qcom_host_reset(hba); >> if (ret) >> @@ -459,7 +475,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) >> return ret; >> } >> >> - phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->phy_gear); >> + phy_set_mode_ext(phy, mode, host->phy_gear); >> >> /* power on phy - start serdes and phy's power and clocks */ >> ret = phy_power_on(phy); >> -- >> 2.7.4 >> >
Hi Mani, On 11/28/2023 2:00 PM, Manivannan Sadhasivam wrote: > On Thu, Nov 23, 2023 at 12:46:27AM -0800, Can Guo wrote: >> Set the initial PHY gear to max HS gear for hosts with HW ver 5 and newer. >> > > MAX_GEAR will be used for hosts with hw_ver.major >= 4 I put it > 5 because I am not intent to touch any old targets which has proven working fine with starting with PHY gear HS_G2. If I put it >= 4, there would be many targets impacted by this change. I need to go back and test those platforms (HW ver == 4). Thanks, Can Guo. > >> This patch is not changing any functionalities or logic but only a >> preparation patch for the next patch in this series. >> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 6756f8d..7bbccf4 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -1067,6 +1067,20 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) >> hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; >> } >> >> +static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) >> +{ >> + struct ufs_host_params *host_params = &host->host_params; >> + >> + 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. > > You need to reword this comment too. > >> + */ >> + if (host->hw_ver.major < 0x5) > > As I mentioned above, MAX_GEAR will be used if hw_ver.major is >=4 in > ufs_qcom_get_hs_gear(). So this check should be (< 0x4). > > - Mani > >> + host->phy_gear = UFS_HS_G2; >> +} >> + >> static void ufs_qcom_set_host_params(struct ufs_hba *hba) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> @@ -1303,6 +1317,7 @@ static int ufs_qcom_init(struct ufs_hba *hba) >> ufs_qcom_set_caps(hba); >> ufs_qcom_advertise_quirks(hba); >> ufs_qcom_set_host_params(hba); >> + ufs_qcom_set_phy_gear(host); >> >> err = ufs_qcom_ice_init(host); >> if (err) >> @@ -1320,12 +1335,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) >> dev_warn(dev, "%s: failed to configure the testbus %d\n", >> __func__, err); >> >> - /* >> - * 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; >> - >> return 0; >> >> out_variant_clear: >> -- >> 2.7.4 >> >
On Tue, Nov 28, 2023 at 03:48:02PM +0800, Can Guo wrote: > Hi Mani, > > On 11/28/2023 1:55 PM, Manivannan Sadhasivam wrote: > > On Thu, Nov 23, 2023 at 12:46:26AM -0800, Can Guo wrote: > > > Qcom UFS hosts, with HW ver 5, can only support up to HS-G5 Rate-A due to > > > HW limitations. If the HS-G5 PHY gear is used, update host_params->hs_rate > > > to Rate-A, so that the subsequent power mode changes shall stick to Rate-A. > > > > > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > One question below... > > > > > --- > > > drivers/ufs/host/ufs-qcom.c | 18 +++++++++++++++++- > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > index 9613ad9..6756f8d 100644 > > > --- a/drivers/ufs/host/ufs-qcom.c > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > @@ -442,9 +442,25 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba) > > > static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > > > { > > > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > > > + struct ufs_host_params *host_params = &host->host_params; > > > struct phy *phy = host->generic_phy; > > > + enum phy_mode mode; > > > int ret; > > > + /* > > > + * HW ver 5 can only support up to HS-G5 Rate-A due to HW limitations. > > > + * If the HS-G5 PHY gear is used, update host_params->hs_rate to Rate-A, > > > + * so that the subsequent power mode change shall stick to Rate-A. > > > + */ > > > + if (host->hw_ver.major == 0x5) { > > > + if (host->phy_gear == UFS_HS_G5) > > > + host_params->hs_rate = PA_HS_MODE_A; > > > + else > > > + host_params->hs_rate = PA_HS_MODE_B; > > > > Is this 'else' part really needed? Since there wouldn't be any 2nd init, I think > > we can skip that. > > We need it because, even there is only one init, if a UFS3.1 device is > attached, phy_gear is given as UFS_HS_G4 in ufs_qcom_set_phy_gear(), hence > we need to put the UFS at HS-G4 Rate B, not Rate A. > But the default hs_rate is PA_HS_MODE_B only and the else condition would be not needed for the 1st init. - Mani > Thanks, > Can Guo. > > > > > - Mani > > > > > + } > > > + > > > + mode = host_params->hs_rate == PA_HS_MODE_B ? PHY_MODE_UFS_HS_B : PHY_MODE_UFS_HS_A; > > > + > > > /* Reset UFS Host Controller and PHY */ > > > ret = ufs_qcom_host_reset(hba); > > > if (ret) > > > @@ -459,7 +475,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > > > return ret; > > > } > > > - phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->phy_gear); > > > + phy_set_mode_ext(phy, mode, host->phy_gear); > > > /* power on phy - start serdes and phy's power and clocks */ > > > ret = phy_power_on(phy); > > > -- > > > 2.7.4 > > > > >
On Tue, Nov 28, 2023 at 03:58:42PM +0800, Can Guo wrote: > Hi Mani, > > On 11/28/2023 2:00 PM, Manivannan Sadhasivam wrote: > > On Thu, Nov 23, 2023 at 12:46:27AM -0800, Can Guo wrote: > > > Set the initial PHY gear to max HS gear for hosts with HW ver 5 and newer. > > > > > > > MAX_GEAR will be used for hosts with hw_ver.major >= 4 > > I put it > 5 because I am not intent to touch any old targets which has > proven working fine with starting with PHY gear HS_G2. If I put it >= 4, > there would be many targets impacted by this change. I need to go back and > test those platforms (HW ver == 4). > This assumption will make the code hard to maintain. I think if you happen to test it on atleast a couple of old targets it should be good since I do not see how others can fail. - Mani > Thanks, > Can Guo. > > > > > > This patch is not changing any functionalities or logic but only a > > > preparation patch for the next patch in this series. > > > > > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > > > --- > > > drivers/ufs/host/ufs-qcom.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > index 6756f8d..7bbccf4 100644 > > > --- a/drivers/ufs/host/ufs-qcom.c > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > @@ -1067,6 +1067,20 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > > > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > > > } > > > +static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) > > > +{ > > > + struct ufs_host_params *host_params = &host->host_params; > > > + > > > + 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. > > > > You need to reword this comment too. > > > > > + */ > > > + if (host->hw_ver.major < 0x5) > > > > As I mentioned above, MAX_GEAR will be used if hw_ver.major is >=4 in > > ufs_qcom_get_hs_gear(). So this check should be (< 0x4). > > > > - Mani > > > > > + host->phy_gear = UFS_HS_G2; > > > +} > > > + > > > static void ufs_qcom_set_host_params(struct ufs_hba *hba) > > > { > > > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > > > @@ -1303,6 +1317,7 @@ static int ufs_qcom_init(struct ufs_hba *hba) > > > ufs_qcom_set_caps(hba); > > > ufs_qcom_advertise_quirks(hba); > > > ufs_qcom_set_host_params(hba); > > > + ufs_qcom_set_phy_gear(host); > > > err = ufs_qcom_ice_init(host); > > > if (err) > > > @@ -1320,12 +1335,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) > > > dev_warn(dev, "%s: failed to configure the testbus %d\n", > > > __func__, err); > > > - /* > > > - * 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; > > > - > > > return 0; > > > out_variant_clear: > > > -- > > > 2.7.4 > > > > >
On 11/28/2023 6:59 PM, Manivannan Sadhasivam wrote: > On Tue, Nov 28, 2023 at 03:58:42PM +0800, Can Guo wrote: >> Hi Mani, >> >> On 11/28/2023 2:00 PM, Manivannan Sadhasivam wrote: >>> On Thu, Nov 23, 2023 at 12:46:27AM -0800, Can Guo wrote: >>>> Set the initial PHY gear to max HS gear for hosts with HW ver 5 and newer. >>>> >>> >>> MAX_GEAR will be used for hosts with hw_ver.major >= 4 >> >> I put it > 5 because I am not intent to touch any old targets which has >> proven working fine with starting with PHY gear HS_G2. If I put it >= 4, >> there would be many targets impacted by this change. I need to go back and >> test those platforms (HW ver == 4). >> > > This assumption will make the code hard to maintain. I think if you happen to > test it on atleast a couple of old targets it should be good since I do not see > how others can fail. Point taken. I will put it >= 4 in next version and test it on platforms like SM8350 and SM8450. Thanks, Can Guo. > > - Mani > >> Thanks, >> Can Guo. >> >>> >>>> This patch is not changing any functionalities or logic but only a >>>> preparation patch for the next patch in this series. >>>> >>>> Signed-off-by: Can Guo <quic_cang@quicinc.com> >>>> --- >>>> drivers/ufs/host/ufs-qcom.c | 21 +++++++++++++++------ >>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>>> index 6756f8d..7bbccf4 100644 >>>> --- a/drivers/ufs/host/ufs-qcom.c >>>> +++ b/drivers/ufs/host/ufs-qcom.c >>>> @@ -1067,6 +1067,20 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) >>>> hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; >>>> } >>>> +static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) >>>> +{ >>>> + struct ufs_host_params *host_params = &host->host_params; >>>> + >>>> + 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. >>> >>> You need to reword this comment too. >>> >>>> + */ >>>> + if (host->hw_ver.major < 0x5) >>> >>> As I mentioned above, MAX_GEAR will be used if hw_ver.major is >=4 in >>> ufs_qcom_get_hs_gear(). So this check should be (< 0x4). >>> >>> - Mani >>> >>>> + host->phy_gear = UFS_HS_G2; >>>> +} >>>> + >>>> static void ufs_qcom_set_host_params(struct ufs_hba *hba) >>>> { >>>> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >>>> @@ -1303,6 +1317,7 @@ static int ufs_qcom_init(struct ufs_hba *hba) >>>> ufs_qcom_set_caps(hba); >>>> ufs_qcom_advertise_quirks(hba); >>>> ufs_qcom_set_host_params(hba); >>>> + ufs_qcom_set_phy_gear(host); >>>> err = ufs_qcom_ice_init(host); >>>> if (err) >>>> @@ -1320,12 +1335,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) >>>> dev_warn(dev, "%s: failed to configure the testbus %d\n", >>>> __func__, err); >>>> - /* >>>> - * 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; >>>> - >>>> return 0; >>>> out_variant_clear: >>>> -- >>>> 2.7.4 >>>> >>> >
On Tue, Nov 28, 2023 at 07:01:27PM +0800, Can Guo wrote: > > > On 11/28/2023 6:59 PM, Manivannan Sadhasivam wrote: > > On Tue, Nov 28, 2023 at 03:58:42PM +0800, Can Guo wrote: > > > Hi Mani, > > > > > > On 11/28/2023 2:00 PM, Manivannan Sadhasivam wrote: > > > > On Thu, Nov 23, 2023 at 12:46:27AM -0800, Can Guo wrote: > > > > > Set the initial PHY gear to max HS gear for hosts with HW ver 5 and newer. > > > > > > > > > > > > > MAX_GEAR will be used for hosts with hw_ver.major >= 4 > > > > > > I put it > 5 because I am not intent to touch any old targets which has > > > proven working fine with starting with PHY gear HS_G2. If I put it >= 4, > > > there would be many targets impacted by this change. I need to go back and > > > test those platforms (HW ver == 4). > > > > > > > This assumption will make the code hard to maintain. I think if you happen to > > test it on atleast a couple of old targets it should be good since I do not see > > how others can fail. > > Point taken. I will put it >= 4 in next version and test it on platforms > like SM8350 and SM8450. > Appreciated, thanks! - Mani > Thanks, > Can Guo. > > > > > - Mani > > > > > Thanks, > > > Can Guo. > > > > > > > > > > > > This patch is not changing any functionalities or logic but only a > > > > > preparation patch for the next patch in this series. > > > > > > > > > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > > > > > --- > > > > > drivers/ufs/host/ufs-qcom.c | 21 +++++++++++++++------ > > > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > > > index 6756f8d..7bbccf4 100644 > > > > > --- a/drivers/ufs/host/ufs-qcom.c > > > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > > > @@ -1067,6 +1067,20 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > > > > > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > > > > > } > > > > > +static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host) > > > > > +{ > > > > > + struct ufs_host_params *host_params = &host->host_params; > > > > > + > > > > > + 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. > > > > > > > > You need to reword this comment too. > > > > > > > > > + */ > > > > > + if (host->hw_ver.major < 0x5) > > > > > > > > As I mentioned above, MAX_GEAR will be used if hw_ver.major is >=4 in > > > > ufs_qcom_get_hs_gear(). So this check should be (< 0x4). > > > > > > > > - Mani > > > > > > > > > + host->phy_gear = UFS_HS_G2; > > > > > +} > > > > > + > > > > > static void ufs_qcom_set_host_params(struct ufs_hba *hba) > > > > > { > > > > > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > > > > > @@ -1303,6 +1317,7 @@ static int ufs_qcom_init(struct ufs_hba *hba) > > > > > ufs_qcom_set_caps(hba); > > > > > ufs_qcom_advertise_quirks(hba); > > > > > ufs_qcom_set_host_params(hba); > > > > > + ufs_qcom_set_phy_gear(host); > > > > > err = ufs_qcom_ice_init(host); > > > > > if (err) > > > > > @@ -1320,12 +1335,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) > > > > > dev_warn(dev, "%s: failed to configure the testbus %d\n", > > > > > __func__, err); > > > > > - /* > > > > > - * 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; > > > > > - > > > > > return 0; > > > > > out_variant_clear: > > > > > -- > > > > > 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. v4 -> v5: Removed two useless debug prints in patch #9 v3 -> v4: 1. 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: Limit negotiated gear to selected PHY gear 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 5 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 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 | 196 ++++++++++++++++++--- 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 | 92 +++++++--- drivers/ufs/host/ufs-qcom.h | 5 +- drivers/ufs/host/ufshcd-pltfrm.c | 69 ++++---- drivers/ufs/host/ufshcd-pltfrm.h | 10 +- 11 files changed, 311 insertions(+), 107 deletions(-)