Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
Hi Kiran,
Thanks for bringing it to my attention. Fortunately I already noticed it
and fixed it. It will be sent soon when all comments on this series are
addressed.
Thanks,
Dolev
Hi Dolev,
On Wednesday 10 September 2014 05:24 PM, Dolev Raviv wrote:
From: Subhash Jadavani subha...@codeaurora.org
This patch adds support for UFS device and UniPro link power management
during runtime/system PM.
snip
+vccq_lpm:
+ufshcd_config_vreg_lpm(hba, hba-vreg_info.vccq);
+vcc_disable:
+ufshcd_toggle_vreg(hba-dev, hba-vreg_info.vcc, false);
+out:
+return ret;
+}
+
+static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba)
+{
+if (ufshcd_is_link_off(hba))
+ufshcd_setup_hba_vreg(hba, false);
I didn't find any declaration and definition of above API in your patch
set.
During compilation of this driver I got a below error, am I missing any
thing?
drivers/scsi/ufs/ufshcd.c: In function ufshcd_hba_vreg_set_lpm:
drivers/scsi/ufs/ufshcd.c:4656:3: error: implicit declaration of
function ufshcd_setup_hba_vreg [-Werror=implicit-function-declaration]
ufshcd_setup_hba_vreg(hba, false);
^
+}
+
+static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba)
+{
+if (ufshcd_is_link_off(hba))
+ufshcd_setup_hba_vreg(hba, true);
Ditto.
}
-EXPORT_SYMBOL_GPL(ufshcd_suspend);
/**
- * ufshcd_resume - resume power management function
+ * ufshcd_suspend - helper function for suspend operations
* @hba: per adapter instance
+ * @pm_op: desired low power operation type
+ *
+ * This function will try to put the UFS device and link into low power
+ * mode based on the rpm_lvl (Runtime PM level) or spm_lvl
+ * (System PM level).
*
snip
+int ufshcd_system_resume(struct ufs_hba *hba)
+{
+if (!hba || !hba-is_powered || pm_runtime_suspended(hba-dev))
+/*
+ * Let the runtime resume take care of resuming
+ * if runtime suspended.
+ */
+return 0;
+else
else is not generally useful after a break or return.
+return ufshcd_resume(hba, UFS_SYSTEM_PM);
+}
+EXPORT_SYMBOL(ufshcd_system_resume);
+
+/**
+ * ufshcd_runtime_suspend - runtime suspend routine
+ * @hba: per adapter instance
+ *
+ * Check the description of ufshcd_suspend() function for more details.
+ *
+ * Returns 0 for success and non-zero for failure
+ */
+int ufshcd_runtime_suspend(struct ufs_hba *hba)
+{
+if (!hba || !hba-is_powered)
+return 0;
+else
else is not generally useful after a break or return.
+return ufshcd_suspend(hba, UFS_RUNTIME_PM);
}
EXPORT_SYMBOL(ufshcd_runtime_suspend);
+/**
+ * ufshcd_runtime_resume - runtime resume routine
+ * @hba: per adapter instance
+ *
+ * This function basically brings the UFS device, UniPro link and
controller
+ * to active state. Following operations are done in this function:
+ *
Thanks,
--Kiran
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
Hi Dolev,
On Wednesday 10 September 2014 05:24 PM, Dolev Raviv wrote:
From: Subhash Jadavani subha...@codeaurora.org
This patch adds support for UFS device and UniPro link power management
during runtime/system PM.
snip
+vccq_lpm:
+ ufshcd_config_vreg_lpm(hba, hba-vreg_info.vccq);
+vcc_disable:
+ ufshcd_toggle_vreg(hba-dev, hba-vreg_info.vcc, false);
+out:
+ return ret;
+}
+
+static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba)
+{
+ if (ufshcd_is_link_off(hba))
+ ufshcd_setup_hba_vreg(hba, false);
I didn't find any declaration and definition of above API in your patch set.
During compilation of this driver I got a below error, am I missing any thing?
drivers/scsi/ufs/ufshcd.c: In function ‘ufshcd_hba_vreg_set_lpm’:
drivers/scsi/ufs/ufshcd.c:4656:3: error: implicit declaration of
function ‘ufshcd_setup_hba_vreg’ [-Werror=implicit-function-declaration]
ufshcd_setup_hba_vreg(hba, false);
^
+}
+
+static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba)
+{
+ if (ufshcd_is_link_off(hba))
+ ufshcd_setup_hba_vreg(hba, true);
Ditto.
}
-EXPORT_SYMBOL_GPL(ufshcd_suspend);
/**
- * ufshcd_resume - resume power management function
+ * ufshcd_suspend - helper function for suspend operations
* @hba: per adapter instance
+ * @pm_op: desired low power operation type
+ *
+ * This function will try to put the UFS device and link into low power
+ * mode based on the rpm_lvl (Runtime PM level) or spm_lvl
+ * (System PM level).
*
snip
+int ufshcd_system_resume(struct ufs_hba *hba)
+{
+ if (!hba || !hba-is_powered || pm_runtime_suspended(hba-dev))
+ /*
+ * Let the runtime resume take care of resuming
+ * if runtime suspended.
+ */
+ return 0;
+ else
else is not generally useful after a break or return.
+ return ufshcd_resume(hba, UFS_SYSTEM_PM);
+}
+EXPORT_SYMBOL(ufshcd_system_resume);
+
+/**
+ * ufshcd_runtime_suspend - runtime suspend routine
+ * @hba: per adapter instance
+ *
+ * Check the description of ufshcd_suspend() function for more details.
+ *
+ * Returns 0 for success and non-zero for failure
+ */
+int ufshcd_runtime_suspend(struct ufs_hba *hba)
+{
+ if (!hba || !hba-is_powered)
+ return 0;
+ else
else is not generally useful after a break or return.
+ return ufshcd_suspend(hba, UFS_RUNTIME_PM);
}
EXPORT_SYMBOL(ufshcd_runtime_suspend);
+/**
+ * ufshcd_runtime_resume - runtime resume routine
+ * @hba: per adapter instance
+ *
+ * This function basically brings the UFS device, UniPro link and controller
+ * to active state. Following operations are done in this function:
+ *
Thanks,
--Kiran
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
2014-09-10 20:54 GMT+09:00 Dolev Raviv dra...@codeaurora.org:
+static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
+ int ua)
+{
+ int ret = 0;
+ struct regulator *reg = vreg-reg;
+ const char *name = vreg-name;
+
+ BUG_ON(!vreg);
+
+ ret = regulator_set_optimum_mode(reg, ua);
+ if (ret = 0) {
+ /*
+* regulator_set_optimum_mode() returns new regulator
+* mode upon success.
+*/
+ ret = 0;
+ } else {
+ dev_err(dev, %s: %s set optimum mode(ua=%d) failed,
err=%d\n,
+ __func__, name, ua, ret);
+ }
+
+ return ret;
+}
+
+static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba,
+struct ufs_vreg *vreg)
+{
+ return ufshcd_config_vreg_load(hba-dev, vreg, UFS_VREG_LPM_LOAD_UA);
+}
If hba-vreg_info.vcc* is NULL as no applicable regulator driver exists,
this function can be called with vreg == NULL through ufshcd_suspend()
- ufshcd_vreg_set_lpm() - ufshcd_config_vreg_lpm(). Then this causes
null pointer dereference or hits BUG_ON in ufshcd_config_vreg_load().
+static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
+struct ufs_vreg *vreg)
+{
+ return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA);
+}
Similar issue as above will happen through ufshcd_resume() -
ufshcd_vreg_set_hpm() - ufshcd_config_vreg_hpm(). Then this causes
null pointer dereference of vreg-max_uA.
So should these functions be noop when vreg == NULL is passed?
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
2014-09-10 20:54 GMT+09:00 Dolev Raviv dra...@codeaurora.org:
+static inline void ufshcd_enable_irq(struct ufs_hba *hba)
+{
+ if (!hba-is_irq_enabled) {
+ enable_irq(hba-irq);
+ hba-is_irq_enabled = true;
+ }
+}
+
+static inline void ufshcd_disable_irq(struct ufs_hba *hba)
+{
+ if (hba-is_irq_enabled) {
+ disable_irq(hba-irq);
+ hba-is_irq_enabled = false;
+ }
+}
This IRQ could be shared among several devices because it is requested
with IRQF_SHARED. So enable_irq()/disable_irq() should be replaced with
request_irq()/free_irq()? Otherwise other devices which share the same
IRQ will be malfunction while disabling IRQ.
Thanks, I will test your suggestion.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
2014-09-10 20:54 GMT+09:00 Dolev Raviv dra...@codeaurora.org:
+static inline void ufshcd_enable_irq(struct ufs_hba *hba)
+{
+ if (!hba-is_irq_enabled) {
+ enable_irq(hba-irq);
+ hba-is_irq_enabled = true;
+ }
+}
+
+static inline void ufshcd_disable_irq(struct ufs_hba *hba)
+{
+ if (hba-is_irq_enabled) {
+ disable_irq(hba-irq);
+ hba-is_irq_enabled = false;
+ }
+}
This IRQ could be shared among several devices because it is requested
with IRQF_SHARED. So enable_irq()/disable_irq() should be replaced with
request_irq()/free_irq()? Otherwise other devices which share the same
IRQ will be malfunction while disabling IRQ.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
