Hi Mani,
On 9/19/2023 6:36 PM, Manivannan Sadhasivam wrote:
On Sun, Sep 10, 2023 at 10:59:22PM -0700, Can Guo wrote:
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@xxxxxxxxxxx>
Signed-off-by: Can Guo <quic_cang@xxxxxxxxxxx>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>
---
drivers/ufs/host/ufs-qcom.c | 27 ++++++++++++++++++---------
drivers/ufs/host/ufs-qcom.h | 1 +
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index c3215d3..710f079 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -443,7 +443,11 @@ 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_dev_params *host_pwr_cap = &host->host_pwr_cap;
struct phy *phy = host->generic_phy;
+ enum phy_mode mode = host_pwr_cap->hs_rate == PA_HS_MODE_B ?
+ PHY_MODE_UFS_HS_B :
+ PHY_MODE_UFS_HS_A;
I do not see anyone passing PA_HS_MODE_A in this patch, so this change is not
required now. If you are doing this as a preparatory work, please do it in a
separate patch.
Sure.
int ret;
/* Reset UFS Host Controller and PHY */
@@ -460,7 +464,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);
Same as above.
Sure.
/* power on phy - start serdes and phy's power and clocks */
ret = phy_power_on(phy);
@@ -884,7 +888,6 @@ 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_dev_params ufs_qcom_cap;
int ret = 0;
if (!dev_req_params) {
@@ -894,13 +897,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
switch (status) {
case PRE_CHANGE:
- ufshcd_init_pwr_dev_param(&ufs_qcom_cap);
- ufs_qcom_cap.hs_rate = UFS_QCOM_LIMIT_HS_RATE;
-
- /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */
- ufs_qcom_cap.hs_tx_gear = ufs_qcom_cap.hs_rx_gear = ufs_qcom_get_hs_gear(hba);
-
- ret = ufshcd_get_pwr_dev_param(&ufs_qcom_cap,
+ ret = ufshcd_get_pwr_dev_param(&host->host_pwr_cap,
dev_max_params,
dev_req_params);
if (ret) {
@@ -1037,6 +1034,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_pwr_mode_limits(struct ufs_hba *hba)
It's good that you are moving the setting to init() as they are static, but I'm
worried about the different naming conventions used all over the place.
The intention here is to set host parameters and then get the agreed one between
host and the device. But different names are being used. The structure itself is
named as "ufs_dev_params" even though it targets host and the vendor drivers are
naming it as "ufs_<vendor>_cap" or "host_cap". And now you've given a new name,
"host_pwr_cap", which makes things even worse.
So we should rename the struct itself as "ufs_host_params" and all the vendor
drivers should stick to "host_params".
I like the name 'ufs_host_params', will unify the declaration of it in
all vendor drivers in next version.
Thanks,
Can Guo.
