| Message ID | 1700729190-17268-7-git-send-email-quic_cang@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,01/10] scsi: ufs: host: Rename structure ufs_dev_params to ufs_host_params | expand |
On 11/23/2023 2:16 PM, 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> > --- > 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; > + } > + > + 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); Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
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. - 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 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 >> >
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 11/28/2023 6:55 PM, Manivannan Sadhasivam wrote: > 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. You are right, but still we need this in case the UFS device version is not populated, meaning dual init can also happen to SM8550. We need to apply the right hs_rate in case the 2nd init asks for HS_G4. Thanks, Can Guo. > > - 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 06:59:39PM +0800, Can Guo wrote: > > > On 11/28/2023 6:55 PM, Manivannan Sadhasivam wrote: > > 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. > > You are right, but still we need this in case the UFS device version is not > populated, meaning dual init can also happen to SM8550. We need to apply the > right hs_rate in case the 2nd init asks for HS_G4. > Hmm, yeah I missed that corner case. This is fine. - Mani > Thanks, > Can Guo. > > > > > - 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 > > > > > > > > > > >
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; + } + + 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);
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> --- drivers/ufs/host/ufs-qcom.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)