Message ID | 1699332374-9324-3-git-send-email-cang@qti.qualcomm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | Enable HS-G5 support on SM8550 | expand |
On Mon, Nov 06, 2023 at 08:46:08PM -0800, Can Guo wrote: > From: Can Guo <quic_cang@quicinc.com> > > Setup host power mode and its limitations during UFS host driver init to > avoid repetitive work during every power mode change. > > Co-developed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> > 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 | 22 ++++++++++++++-------- > drivers/ufs/host/ufs-qcom.h | 1 + > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index aee66a3..cc0eb37 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -898,7 +898,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > struct ufs_pa_layer_attr *dev_req_params) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > - struct ufs_host_params host_params; > + struct ufs_host_params *host_params = &host->host_params; > int ret = 0; > > if (!dev_req_params) { > @@ -908,13 +908,7 @@ 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; You drop the setting of hs_rate in your new function. It seems setting that's also overkill since UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B. hs_rate is already set to that in ufshcd_init_host_param(), so removing it makes sense. Can you remove it prior in its own patch, and remove the now unused UFS_QCOM_LIMIT_HS_RATE as well from ufs-qcom.h? With that in place this seems like a good improvement: Acked-by: Andrew Halaney <ahalaney@redhat.com> > - > - /* 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); > - > - ret = ufshcd_negotiate_pwr_param(&host_params, dev_max_params, dev_req_params); > + ret = ufshcd_negotiate_pwr_param(host_params, dev_max_params, dev_req_params); > if (ret) { > dev_err(hba->dev, "%s: failed to determine capabilities\n", > __func__); > @@ -1049,6 +1043,17 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > } > > +static void ufs_qcom_set_host_params(struct ufs_hba *hba) > +{ > + struct ufs_qcom_host *host = ufshcd_get_variant(hba); > + struct ufs_host_params *host_params = &host->host_params; > + > + ufshcd_init_host_param(host_params); > + > + /* 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); > +} > + > static void ufs_qcom_set_caps(struct ufs_hba *hba) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > @@ -1273,6 +1278,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); > > err = ufs_qcom_ice_init(host); > if (err) > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 9950a00..ab94c54 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -240,6 +240,7 @@ struct ufs_qcom_host { > > struct gpio_desc *device_reset; > > + struct ufs_host_params host_params; > u32 phy_gear; > > bool esi_enabled; > -- > 2.7.4 > >
On Mon, Nov 06, 2023 at 08:46:08PM -0800, Can Guo wrote: > From: Can Guo <quic_cang@quicinc.com> > > Setup host power mode and its limitations during UFS host driver init to > avoid repetitive work during every power mode change. > > Co-developed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> > 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 | 22 ++++++++++++++-------- > drivers/ufs/host/ufs-qcom.h | 1 + > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index aee66a3..cc0eb37 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -898,7 +898,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > struct ufs_pa_layer_attr *dev_req_params) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > - struct ufs_host_params host_params; > + struct ufs_host_params *host_params = &host->host_params; > int ret = 0; > > if (!dev_req_params) { > @@ -908,13 +908,7 @@ 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; As Andrew spotted, this gets removed without explanation. So, I'd also suggest doing it in a separate patch. - Mani > - > - /* 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); > - > - ret = ufshcd_negotiate_pwr_param(&host_params, dev_max_params, dev_req_params); > + ret = ufshcd_negotiate_pwr_param(host_params, dev_max_params, dev_req_params); > if (ret) { > dev_err(hba->dev, "%s: failed to determine capabilities\n", > __func__); > @@ -1049,6 +1043,17 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > } > > +static void ufs_qcom_set_host_params(struct ufs_hba *hba) > +{ > + struct ufs_qcom_host *host = ufshcd_get_variant(hba); > + struct ufs_host_params *host_params = &host->host_params; > + > + ufshcd_init_host_param(host_params); > + > + /* 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); > +} > + > static void ufs_qcom_set_caps(struct ufs_hba *hba) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > @@ -1273,6 +1278,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); > > err = ufs_qcom_ice_init(host); > if (err) > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 9950a00..ab94c54 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -240,6 +240,7 @@ struct ufs_qcom_host { > > struct gpio_desc *device_reset; > > + struct ufs_host_params host_params; > u32 phy_gear; > > bool esi_enabled; > -- > 2.7.4 >
Hi Andrew, On 11/8/2023 4:14 AM, Andrew Halaney wrote: > On Mon, Nov 06, 2023 at 08:46:08PM -0800, Can Guo wrote: >> From: Can Guo <quic_cang@quicinc.com> >> >> Setup host power mode and its limitations during UFS host driver init to >> avoid repetitive work during every power mode change. >> >> Co-developed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> >> 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 | 22 ++++++++++++++-------- >> drivers/ufs/host/ufs-qcom.h | 1 + >> 2 files changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index aee66a3..cc0eb37 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -898,7 +898,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, >> struct ufs_pa_layer_attr *dev_req_params) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> - struct ufs_host_params host_params; >> + struct ufs_host_params *host_params = &host->host_params; >> int ret = 0; >> >> if (!dev_req_params) { >> @@ -908,13 +908,7 @@ 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; > You drop the setting of hs_rate in your new function. It seems setting that's > also overkill since UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B. hs_rate is > already set to that in ufshcd_init_host_param(), so removing it makes > sense. > > Can you remove it prior in its own patch, and remove the now unused > UFS_QCOM_LIMIT_HS_RATE as well from ufs-qcom.h? Sure, will address this in next version. > > With that in place this seems like a good improvement: > > Acked-by: Andrew Halaney <ahalaney@redhat.com> Thanks, Can Guo.
Hi Mani, On 11/8/2023 1:07 PM, Manivannan Sadhasivam wrote: > On Mon, Nov 06, 2023 at 08:46:08PM -0800, Can Guo wrote: >> From: Can Guo <quic_cang@quicinc.com> >> >> Setup host power mode and its limitations during UFS host driver init to >> avoid repetitive work during every power mode change. >> >> Co-developed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> >> 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 | 22 ++++++++++++++-------- >> drivers/ufs/host/ufs-qcom.h | 1 + >> 2 files changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index aee66a3..cc0eb37 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -898,7 +898,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, >> struct ufs_pa_layer_attr *dev_req_params) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> - struct ufs_host_params host_params; >> + struct ufs_host_params *host_params = &host->host_params; >> int ret = 0; >> >> if (!dev_req_params) { >> @@ -908,13 +908,7 @@ 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; > > As Andrew spotted, this gets removed without explanation. So, I'd also suggest > doing it in a separate patch. > > - Mani Sure, will do Thanks, Can Guo.
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index aee66a3..cc0eb37 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -898,7 +898,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, struct ufs_pa_layer_attr *dev_req_params) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); - struct ufs_host_params host_params; + struct ufs_host_params *host_params = &host->host_params; int ret = 0; if (!dev_req_params) { @@ -908,13 +908,7 @@ 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); - - ret = ufshcd_negotiate_pwr_param(&host_params, dev_max_params, dev_req_params); + ret = ufshcd_negotiate_pwr_param(host_params, dev_max_params, dev_req_params); if (ret) { dev_err(hba->dev, "%s: failed to determine capabilities\n", __func__); @@ -1049,6 +1043,17 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; } +static void ufs_qcom_set_host_params(struct ufs_hba *hba) +{ + struct ufs_qcom_host *host = ufshcd_get_variant(hba); + struct ufs_host_params *host_params = &host->host_params; + + ufshcd_init_host_param(host_params); + + /* 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); +} + static void ufs_qcom_set_caps(struct ufs_hba *hba) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); @@ -1273,6 +1278,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); err = ufs_qcom_ice_init(host); if (err) diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index 9950a00..ab94c54 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -240,6 +240,7 @@ struct ufs_qcom_host { struct gpio_desc *device_reset; + struct ufs_host_params host_params; u32 phy_gear; bool esi_enabled;