Add CPU latency QoS support for ufs driver. This improves random io
performance by 15% for ufs.
tiotest benchmark tool io performance results on sm8550 platform:
1. Without PM QoS support
Type (Speed in) | Average of 18 iterations
Random Read(IPOS) | 37101.3
Random Write(IPOS) | 41065.13
2. With PM QoS support
Type (Speed in) | Average of 18 iterations
Random Read(IPOS) | 42943.4
Random Write(IPOS) | 46784.9
(Improvement % with PM QoS = ~15%).
This patch is based on below patch by Stanley Chu [1].
Moving the PM QoS code to ufshcd.c and making it generic.
[1] https://lore.kernel.org/r/[email protected]
Changes from v1:
- Addressed bvanassche comments to have the code in core ufshcd
- Design is changed from per-device PM QoS to CPU latency QoS based support
- Reverted existing PM QoS feature from MEDIATEK UFS driver
- Added PM QoS capability for both QCOM and MEDIATEK SoCs
Maramaina Naresh (3):
ufs: core: Add CPU latency QoS support for ufs driver
ufs: ufs-mediatek: Enable CPU latency PM QoS support for MEDIATEK SoC
ufs: ufs-qcom: Enable CPU latency QoS support for QCOM SoC
drivers/ufs/core/ufshcd-priv.h | 8 +++++
drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++++++++++++++
drivers/ufs/host/ufs-mediatek.c | 20 ++---------
drivers/ufs/host/ufs-mediatek.h | 3 --
drivers/ufs/host/ufs-qcom.c | 1 +
include/ufs/ufshcd.h | 16 +++++++++
6 files changed, 90 insertions(+), 20 deletions(-)
--
2.34.1
Revert the existing PM QoS feature from MEDIATEK UFS driver as similar
PM QoS feature implementation is moved to core ufshcd and also enable
CPU latency PM QoS capability for MEDIATEK SoC.
Signed-off-by: Maramaina Naresh <[email protected]>
---
drivers/ufs/host/ufs-mediatek.c | 20 +++-----------------
drivers/ufs/host/ufs-mediatek.h | 3 ---
2 files changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index fc61790d289b..d8ece88103b9 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -17,7 +17,6 @@
#include <linux/of_platform.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
-#include <linux/pm_qos.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/soc/mediatek/mtk_sip_svc.h>
@@ -626,21 +625,9 @@ static void ufs_mtk_init_host_caps(struct ufs_hba *hba)
dev_info(hba->dev, "caps: 0x%x", host->caps);
}
-static void ufs_mtk_boost_pm_qos(struct ufs_hba *hba, bool boost)
-{
- struct ufs_mtk_host *host = ufshcd_get_variant(hba);
-
- if (!host || !host->pm_qos_init)
- return;
-
- cpu_latency_qos_update_request(&host->pm_qos_req,
- boost ? 0 : PM_QOS_DEFAULT_VALUE);
-}
-
static void ufs_mtk_scale_perf(struct ufs_hba *hba, bool scale_up)
{
ufs_mtk_boost_crypt(hba, scale_up);
- ufs_mtk_boost_pm_qos(hba, scale_up);
}
static void ufs_mtk_pwr_ctrl(struct ufs_hba *hba, bool on)
@@ -937,6 +924,9 @@ static int ufs_mtk_init(struct ufs_hba *hba)
/* Enable clk scaling*/
hba->caps |= UFSHCD_CAP_CLK_SCALING;
+ /* Enable PM QoS */
+ hba->caps |= UFSHCD_CAP_PM_QOS;
+
hba->quirks |= UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL;
hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_INTR;
hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_RTC;
@@ -959,10 +949,6 @@ static int ufs_mtk_init(struct ufs_hba *hba)
host->ip_ver = ufshcd_readl(hba, REG_UFS_MTK_IP_VER);
- /* Initialize pm-qos request */
- cpu_latency_qos_add_request(&host->pm_qos_req, PM_QOS_DEFAULT_VALUE);
- host->pm_qos_init = true;
-
goto out;
out_variant_clear:
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index f76e80d91729..38eab95b0f79 100644
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -7,7 +7,6 @@
#define _UFS_MEDIATEK_H
#include <linux/bitops.h>
-#include <linux/pm_qos.h>
#include <linux/soc/mediatek/mtk_sip_svc.h>
/*
@@ -167,7 +166,6 @@ struct ufs_mtk_mcq_intr_info {
struct ufs_mtk_host {
struct phy *mphy;
- struct pm_qos_request pm_qos_req;
struct regulator *reg_va09;
struct reset_control *hci_reset;
struct reset_control *unipro_reset;
@@ -178,7 +176,6 @@ struct ufs_mtk_host {
struct ufs_mtk_hw_ver hw_ver;
enum ufs_mtk_host_caps caps;
bool mphy_powered_on;
- bool pm_qos_init;
bool unipro_lpm;
bool ref_clk_enabled;
u16 ref_clk_ungating_wait_us;
--
2.34.1
Register ufs driver to CPU latency PM QoS framework can improves
ufs device random io performance.
PM QoS initialization will insert new QoS request into the CPU
latency QoS list with the maximum latency PM_QOS_DEFAULT_VALUE
value.
UFS driver will vote for performance mode on scale up and power
save mode for scale down.
If clock scaling feature is not enabled then voting will be based
on clock on or off condition.
tiotest benchmark tool io performance results on sm8550 platform:
1. Without PM QoS support
Type (Speed in) | Average of 18 iterations
Random Write(IPOS) | 41065.13
Random Read(IPOS) | 37101.3
2. With PM QoS support
Type (Speed in) | Average of 18 iterations
Random Write(IPOS) | 46784.9
Random Read(IPOS) | 42943.4
(Improvement % with PM QoS = ~15%).
Co-developed-by: Nitin Rawat <[email protected]>
Signed-off-by: Nitin Rawat <[email protected]>
Signed-off-by: Naveen Kumar Goud Arepalli <[email protected]>
Signed-off-by: Maramaina Naresh <[email protected]>
---
drivers/ufs/core/ufshcd-priv.h | 8 +++++
drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++++++++++++++++
include/ufs/ufshcd.h | 16 +++++++++
3 files changed, 86 insertions(+)
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..536805f6c4e1 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -241,6 +241,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
hba->vops->config_scaling_param(hba, p, data);
}
+static inline u32 ufshcd_vops_config_qos_vote(struct ufs_hba *hba)
+{
+ if (hba->vops && hba->vops->config_qos_vote)
+ return hba->vops->config_qos_vote(hba);
+
+ return UFSHCD_QOS_DEFAULT_VOTE;
+}
+
static inline void ufshcd_vops_reinit_notify(struct ufs_hba *hba)
{
if (hba->vops && hba->vops->reinit_notify)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ae9936fc6ffb..13370febd2b5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1001,6 +1001,20 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
return ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6;
}
+/**
+ * ufshcd_pm_qos_perf - vote for PM QoS performance or power save mode
+ * @hba: per adapter instance
+ * @on: If True, vote for perf PM QoS mode otherwise power save mode
+ */
+static void ufshcd_pm_qos_perf(struct ufs_hba *hba, bool on)
+{
+ if (!hba->pm_qos_init)
+ return;
+
+ cpu_latency_qos_update_request(&hba->pm_qos_req, on ? hba->qos_vote
+ : PM_QOS_DEFAULT_VALUE);
+}
+
/**
* ufshcd_set_clk_freq - set UFS controller clock frequencies
* @hba: per adapter instance
@@ -1153,6 +1167,10 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
(scale_up ? "up" : "down"),
ktime_to_us(ktime_sub(ktime_get(), start)), ret);
+
+ if (!ret)
+ ufshcd_pm_qos_perf(hba, scale_up);
+
return ret;
}
@@ -9204,6 +9222,8 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
if (ret)
return ret;
+ if (!ufshcd_is_clkscaling_supported(hba))
+ ufshcd_pm_qos_perf(hba, on);
out:
if (ret) {
list_for_each_entry(clki, head, list) {
@@ -9296,6 +9316,45 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
return ret;
}
+/**
+ * ufshcd_pm_qos_init - initialize PM QoS instance
+ * @hba: per adapter instance
+ */
+static void ufshcd_pm_qos_init(struct ufs_hba *hba)
+{
+ if (!(hba->caps & UFSHCD_CAP_PM_QOS))
+ return;
+
+ /*
+ * called to configure PM QoS vote value for UFS host,
+ * expecting qos vote return value from caller else
+ * default vote value will be return.
+ */
+ hba->qos_vote = ufshcd_vops_config_qos_vote(hba);
+ cpu_latency_qos_add_request(&hba->pm_qos_req,
+ PM_QOS_DEFAULT_VALUE);
+
+ if (cpu_latency_qos_request_active(&hba->pm_qos_req))
+ hba->pm_qos_init = true;
+
+ dev_dbg(hba->dev, "%s: QoS %s, qos_vote: %u\n", __func__,
+ hba->pm_qos_init ? "initialized" : "uninitialized",
+ hba->qos_vote);
+}
+
+/**
+ * ufshcd_pm_qos_exit - remove instance from PM QoS
+ * @hba: per adapter instance
+ */
+static void ufshcd_pm_qos_exit(struct ufs_hba *hba)
+{
+ if (!hba->pm_qos_init)
+ return;
+
+ cpu_latency_qos_remove_request(&hba->pm_qos_req);
+ hba->pm_qos_init = false;
+}
+
static int ufshcd_variant_hba_init(struct ufs_hba *hba)
{
int err = 0;
@@ -9381,6 +9440,7 @@ static int ufshcd_hba_init(struct ufs_hba *hba)
static void ufshcd_hba_exit(struct ufs_hba *hba)
{
if (hba->is_powered) {
+ ufshcd_pm_qos_exit(hba);
ufshcd_exit_clk_scaling(hba);
ufshcd_exit_clk_gating(hba);
if (hba->eh_wq)
@@ -10030,6 +10090,7 @@ static int ufshcd_suspend(struct ufs_hba *hba)
ufshcd_vreg_set_lpm(hba);
/* Put the host controller in low power mode if possible */
ufshcd_hba_vreg_set_lpm(hba);
+ ufshcd_pm_qos_perf(hba, false);
return ret;
}
@@ -10576,6 +10637,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
ufs_sysfs_add_nodes(hba->dev);
device_enable_async_suspend(dev);
+ ufshcd_pm_qos_init(hba);
return 0;
free_tmf_queue:
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d862c8ddce03..e9f2bad8934e 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -320,6 +320,9 @@ struct ufs_pwr_mode_info {
* @phy_initialization: used to initialize phys
* @device_reset: called to issue a reset pulse on the UFS device
* @config_scaling_param: called to configure clock scaling parameters
+ * @config_qos_vote: called to configure PM QoS vote value for UFS host,
+ * expecting qos vote return value from caller else
+ default vote value will be return
* @program_key: program or evict an inline encryption key
* @event_notify: called to notify important events
* @reinit_notify: called to notify reinit of UFSHCD during max gear switch
@@ -362,6 +365,7 @@ struct ufs_hba_variant_ops {
void (*config_scaling_param)(struct ufs_hba *hba,
struct devfreq_dev_profile *profile,
struct devfreq_simple_ondemand_data *data);
+ u32 (*config_qos_vote)(struct ufs_hba *hba);
int (*program_key)(struct ufs_hba *hba,
const union ufs_crypto_cfg_entry *cfg, int slot);
void (*event_notify)(struct ufs_hba *hba,
@@ -720,6 +724,11 @@ enum ufshcd_caps {
* WriteBooster when scaling the clock down.
*/
UFSHCD_CAP_WB_WITH_CLK_SCALING = 1 << 12,
+
+ /* This capability allows the host controller driver to use the PM QoS
+ * feature.
+ */
+ UFSHCD_CAP_PM_QOS = 1 << 13,
};
struct ufs_hba_variant_params {
@@ -793,6 +802,8 @@ enum ufshcd_mcq_opr {
OPR_MAX,
};
+#define UFSHCD_QOS_DEFAULT_VOTE 0
+
/**
* struct ufs_hba - per adapter private structure
* @mmio_base: UFSHCI base register address
@@ -912,6 +923,8 @@ enum ufshcd_mcq_opr {
* @mcq_base: Multi circular queue registers base address
* @uhq: array of supported hardware queues
* @dev_cmd_queue: Queue for issuing device management commands
+ * @pm_qos_req: PM QoS request handle
+ * @pm_qos_init: flag to check if pm qos init completed
*/
struct ufs_hba {
void __iomem *mmio_base;
@@ -1076,6 +1089,9 @@ struct ufs_hba {
struct ufs_hw_queue *uhq;
struct ufs_hw_queue *dev_cmd_queue;
struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
+ struct pm_qos_request pm_qos_req;
+ bool pm_qos_init;
+ u32 qos_vote;
};
/**
--
2.34.1
On 12/4/23 06:30, Maramaina Naresh wrote:
> + u32 (*config_qos_vote)(struct ufs_hba *hba);
Please remove the above callback since this patch series does not
introduce any instances of this callback.
> +
> + /* This capability allows the host controller driver to use the PM QoS
> + * feature.
> + */
> + UFSHCD_CAP_PM_QOS = 1 << 13,
> };
Why does it depend on the host driver whether or not PM QoS is
enabled? Why isn't it enabled unconditionally?
> + * @pm_qos_req: PM QoS request handle
> + * @pm_qos_init: flag to check if pm qos init completed
> */
Documentation for pm_qos_init is missing.
> struct ufs_hba {
> void __iomem *mmio_base;
> @@ -1076,6 +1089,9 @@ struct ufs_hba {
> struct ufs_hw_queue *uhq;
> struct ufs_hw_queue *dev_cmd_queue;
> struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
> + struct pm_qos_request pm_qos_req;
> + bool pm_qos_init;
> + u32 qos_vote;
Please rename "pm_qos_init" into "pm_qos_initialized".
Thanks,
Bart.
On 12/5/2023 12:30 AM, Bart Van Assche wrote:
> On 12/4/23 06:30, Maramaina Naresh wrote:
>> + u32 (*config_qos_vote)(struct ufs_hba *hba);
>
> Please remove the above callback since this patch series does not
> introduce any instances of this callback.
>
Sure Bart, i will take care of this comment in next patch set.
If some SoC vendor have a different qos vote value then this callback
can be added in future.
>> +
>> + /* This capability allows the host controller driver to use the
>> PM QoS
>> + * feature.
>> + */
>> + UFSHCD_CAP_PM_QOS = 1 << 13,
>> };
>
> Why does it depend on the host driver whether or not PM QoS is
> enabled? Why isn't it enabled unconditionally?
For some platform vendors power KPI might be more important than random
io KPI. Hence this flag is disabled by default and can be enabled based
on platform requirement.
>
>> + * @pm_qos_req: PM QoS request handle
>> + * @pm_qos_init: flag to check if pm qos init completed
>> */
>
> Documentation for pm_qos_init is missing.
>
Sorry, i didn't get your comment, i have already added documentation for
@pm_qos_init, @pm_qos_req variable as above. Do you want me to add this
information some where else as well?
>> struct ufs_hba {
>> void __iomem *mmio_base;
>> @@ -1076,6 +1089,9 @@ struct ufs_hba {
>> struct ufs_hw_queue *uhq;
>> struct ufs_hw_queue *dev_cmd_queue;
>> struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
>> + struct pm_qos_request pm_qos_req;
>> + bool pm_qos_init;
>> + u32 qos_vote;
>
> Please rename "pm_qos_init" into "pm_qos_initialized".
>
Sure Bart, i will take care of this comment in next patch set.
> Thanks,
>
> Bart.
>
Thanks,
Naresh.
On 12/4/23 21:58, Naresh Maramaina wrote:
> On 12/5/2023 12:30 AM, Bart Van Assche wrote:
>> On 12/4/23 06:30, Maramaina Naresh wrote:
>>> + /* This capability allows the host controller driver to use the
>>> PM QoS
>>> + * feature.
>>> + */
>>> + UFSHCD_CAP_PM_QOS = 1 << 13,
>>> };
>>
>> Why does it depend on the host driver whether or not PM QoS is
>> enabled? Why isn't it enabled unconditionally?
>
> For some platform vendors power KPI might be more important than random
> io KPI. Hence this flag is disabled by default and can be enabled based
> on platform requirement.
How about leaving this flag out unless if a host vendor asks explicitly
for this flag?
>>
>>> + * @pm_qos_req: PM QoS request handle
>>> + * @pm_qos_init: flag to check if pm qos init completed
>>> */
>>
>> Documentation for pm_qos_init is missing.
>>
> Sorry, i didn't get your comment, i have already added documentation for
> @pm_qos_init, @pm_qos_req variable as above. Do you want me to add this
> information some where else as well?
Oops, I meant 'qos_vote'.
Thanks,
Bart.
On 12/5/2023 10:41 PM, Bart Van Assche wrote:
> On 12/4/23 21:58, Naresh Maramaina wrote:
>> On 12/5/2023 12:30 AM, Bart Van Assche wrote:
>>> On 12/4/23 06:30, Maramaina Naresh wrote:
>>>> + /* This capability allows the host controller driver to use the
>>>> PM QoS
>>>> + * feature.
>>>> + */
>>>> + UFSHCD_CAP_PM_QOS = 1 << 13,
>>>> };
>>>
>>> Why does it depend on the host driver whether or not PM QoS is
>>> enabled? Why isn't it enabled unconditionally?
>>
>> For some platform vendors power KPI might be more important than
>> random io KPI. Hence this flag is disabled by default and can be
>> enabled based on platform requirement.
>
> How about leaving this flag out unless if a host vendor asks explicitly
> for this flag?
IMHO, instead of completely removing this flag, how about having
flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
by default and if some host vendor wants to disable it explicitly,
they can enable that flag.
Please let me know your opinion.
>>>
>>>> + * @pm_qos_req: PM QoS request handle
>>>> + * @pm_qos_init: flag to check if pm qos init completed
>>>> */
>>>
>>> Documentation for pm_qos_init is missing.
>>>
>> Sorry, i didn't get your comment, i have already added documentation
>> for @pm_qos_init, @pm_qos_req variable as above. Do you want me to add
>> this information some where else as well?
>
> Oops, I meant 'qos_vote'.
Sure. I'll take of this in next patchset.
>
> Thanks,
>
> Bart.
>
Thanks,
Naresh
On Mon, Dec 04, 2023 at 08:00:59PM +0530, Maramaina Naresh wrote:
> Register ufs driver to CPU latency PM QoS framework can improves
> ufs device random io performance.
>
> PM QoS initialization will insert new QoS request into the CPU
> latency QoS list with the maximum latency PM_QOS_DEFAULT_VALUE
> value.
>
> UFS driver will vote for performance mode on scale up and power
> save mode for scale down.
>
> If clock scaling feature is not enabled then voting will be based
> on clock on or off condition.
>
> tiotest benchmark tool io performance results on sm8550 platform:
>
> 1. Without PM QoS support
> Type (Speed in) | Average of 18 iterations
> Random Write(IPOS) | 41065.13
> Random Read(IPOS) | 37101.3
>
> 2. With PM QoS support
> Type (Speed in) | Average of 18 iterations
> Random Write(IPOS) | 46784.9
> Random Read(IPOS) | 42943.4
> (Improvement % with PM QoS = ~15%).
>
> Co-developed-by: Nitin Rawat <[email protected]>
> Signed-off-by: Nitin Rawat <[email protected]>
> Signed-off-by: Naveen Kumar Goud Arepalli <[email protected]>
> Signed-off-by: Maramaina Naresh <[email protected]>
> ---
> drivers/ufs/core/ufshcd-priv.h | 8 +++++
> drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++++++++++++++++
> include/ufs/ufshcd.h | 16 +++++++++
> 3 files changed, 86 insertions(+)
>
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index f42d99ce5bf1..536805f6c4e1 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -241,6 +241,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
> hba->vops->config_scaling_param(hba, p, data);
> }
>
> +static inline u32 ufshcd_vops_config_qos_vote(struct ufs_hba *hba)
> +{
> + if (hba->vops && hba->vops->config_qos_vote)
> + return hba->vops->config_qos_vote(hba);
Please remove this callback as Bart noted.
> +
> + return UFSHCD_QOS_DEFAULT_VOTE;
> +}
> +
> static inline void ufshcd_vops_reinit_notify(struct ufs_hba *hba)
> {
> if (hba->vops && hba->vops->reinit_notify)
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ae9936fc6ffb..13370febd2b5 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1001,6 +1001,20 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
> return ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6;
> }
>
> +/**
> + * ufshcd_pm_qos_perf - vote for PM QoS performance or power save mode
ufshcd_pm_qos_update() - Update PM QoS request
> + * @hba: per adapter instance
> + * @on: If True, vote for perf PM QoS mode otherwise power save mode
> + */
> +static void ufshcd_pm_qos_perf(struct ufs_hba *hba, bool on)
> +{
> + if (!hba->pm_qos_init)
> + return;
> +
> + cpu_latency_qos_update_request(&hba->pm_qos_req, on ? hba->qos_vote
> + : PM_QOS_DEFAULT_VALUE);
> +}
> +
> /**
> * ufshcd_set_clk_freq - set UFS controller clock frequencies
> * @hba: per adapter instance
> @@ -1153,6 +1167,10 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
> trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
> (scale_up ? "up" : "down"),
> ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> +
> + if (!ret)
> + ufshcd_pm_qos_perf(hba, scale_up);
Can't you just move this before trace_ufshcd_profile_clk_scaling()? This also
avoids checking for !ret.
> +
> return ret;
> }
>
> @@ -9204,6 +9222,8 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
> if (ret)
> return ret;
>
> + if (!ufshcd_is_clkscaling_supported(hba))
> + ufshcd_pm_qos_perf(hba, on);
> out:
> if (ret) {
> list_for_each_entry(clki, head, list) {
> @@ -9296,6 +9316,45 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
> return ret;
> }
>
> +/**
> + * ufshcd_pm_qos_init - initialize PM QoS instance
"Initialize PM QoS request"
> + * @hba: per adapter instance
> + */
> +static void ufshcd_pm_qos_init(struct ufs_hba *hba)
> +{
> + if (!(hba->caps & UFSHCD_CAP_PM_QOS))
> + return;
> +
> + /*
> + * called to configure PM QoS vote value for UFS host,
> + * expecting qos vote return value from caller else
> + * default vote value will be return.
> + */
> + hba->qos_vote = ufshcd_vops_config_qos_vote(hba);
No need of this variable too if you get rid of the callback.
> + cpu_latency_qos_add_request(&hba->pm_qos_req,
> + PM_QOS_DEFAULT_VALUE);
> +
> + if (cpu_latency_qos_request_active(&hba->pm_qos_req))
> + hba->pm_qos_init = true;
Why do you need this flag?
> +
> + dev_dbg(hba->dev, "%s: QoS %s, qos_vote: %u\n", __func__,
> + hba->pm_qos_init ? "initialized" : "uninitialized",
> + hba->qos_vote);
> +}
> +
> +/**
> + * ufshcd_pm_qos_exit - remove instance from PM QoS
> + * @hba: per adapter instance
> + */
> +static void ufshcd_pm_qos_exit(struct ufs_hba *hba)
> +{
> + if (!hba->pm_qos_init)
> + return;
> +
> + cpu_latency_qos_remove_request(&hba->pm_qos_req);
> + hba->pm_qos_init = false;
> +}
> +
[...]
> /**
> * struct ufs_hba - per adapter private structure
> * @mmio_base: UFSHCI base register address
> @@ -912,6 +923,8 @@ enum ufshcd_mcq_opr {
> * @mcq_base: Multi circular queue registers base address
> * @uhq: array of supported hardware queues
> * @dev_cmd_queue: Queue for issuing device management commands
> + * @pm_qos_req: PM QoS request handle
> + * @pm_qos_init: flag to check if pm qos init completed
> */
> struct ufs_hba {
> void __iomem *mmio_base;
> @@ -1076,6 +1089,9 @@ struct ufs_hba {
> struct ufs_hw_queue *uhq;
> struct ufs_hw_queue *dev_cmd_queue;
> struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
> + struct pm_qos_request pm_qos_req;
> + bool pm_qos_init;
> + u32 qos_vote;
Order doesn't match Kdoc.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
>
>
> On 12/5/2023 10:41 PM, Bart Van Assche wrote:
> > On 12/4/23 21:58, Naresh Maramaina wrote:
> > > On 12/5/2023 12:30 AM, Bart Van Assche wrote:
> > > > On 12/4/23 06:30, Maramaina Naresh wrote:
> > > > > + /* This capability allows the host controller driver to
> > > > > use the PM QoS
> > > > > + * feature.
> > > > > + */
> > > > > + UFSHCD_CAP_PM_QOS = 1 << 13,
> > > > > };
> > > >
> > > > Why does it depend on the host driver whether or not PM QoS is
> > > > enabled? Why isn't it enabled unconditionally?
> > >
> > > For some platform vendors power KPI might be more important than
> > > random io KPI. Hence this flag is disabled by default and can be
> > > enabled based on platform requirement.
> >
> > How about leaving this flag out unless if a host vendor asks explicitly
> > for this flag?
>
> IMHO, instead of completely removing this flag, how about having
> flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
> by default and if some host vendor wants to disable it explicitly,
> they can enable that flag.
> Please let me know your opinion.
>
If a vendor wants to disable this feature, then the driver has to be modified.
That won't be very convenient. So either this has to be configured through sysfs
or Kconfig if flexibility matters.
- Mani
> > > >
> > > > > + * @pm_qos_req: PM QoS request handle
> > > > > + * @pm_qos_init: flag to check if pm qos init completed
> > > > > */
> > > >
> > > > Documentation for pm_qos_init is missing.
> > > >
> > > Sorry, i didn't get your comment, i have already added documentation
> > > for @pm_qos_init, @pm_qos_req variable as above. Do you want me to
> > > add this information some where else as well?
> >
> > Oops, I meant 'qos_vote'.
>
> Sure. I'll take of this in next patchset.
>
> >
> > Thanks,
> >
> > Bart.
> >
>
> Thanks,
> Naresh
>
--
மணிவண்ணன் சதாசிவம்
On 12/6/23 05:32, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
>> On 12/5/2023 10:41 PM, Bart Van Assche wrote:
>>> On 12/4/23 21:58, Naresh Maramaina wrote:
>>>> On 12/5/2023 12:30 AM, Bart Van Assche wrote:
>>>>> On 12/4/23 06:30, Maramaina Naresh wrote:
>>>>>> + /* This capability allows the host controller driver to
>>>>>> use the PM QoS
>>>>>> + * feature.
>>>>>> + */
>>>>>> + UFSHCD_CAP_PM_QOS = 1 << 13,
>>>>>> };
>>>>>
>>>>> Why does it depend on the host driver whether or not PM QoS is
>>>>> enabled? Why isn't it enabled unconditionally?
>>>>
>>>> For some platform vendors power KPI might be more important than
>>>> random io KPI. Hence this flag is disabled by default and can be
>>>> enabled based on platform requirement.
>>>
>>> How about leaving this flag out unless if a host vendor asks explicitly
>>> for this flag?
>>
>> IMHO, instead of completely removing this flag, how about having
>> flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
>> by default and if some host vendor wants to disable it explicitly,
>> they can enable that flag.
>> Please let me know your opinion.
That would result in a flag that is tested but that is never set by
upstream code. I'm not sure that's acceptable.
> If a vendor wants to disable this feature, then the driver has to be modified.
> That won't be very convenient. So either this has to be configured through sysfs
> or Kconfig if flexibility matters.
Kconfig sounds worse to me because changing any Kconfig flag requires a
modification of the Android GKI kernel.
Thanks,
Bart.
On Wed, Dec 06, 2023 at 03:02:04PM -1000, Bart Van Assche wrote:
> On 12/6/23 05:32, Manivannan Sadhasivam wrote:
> > On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
> > > On 12/5/2023 10:41 PM, Bart Van Assche wrote:
> > > > On 12/4/23 21:58, Naresh Maramaina wrote:
> > > > > On 12/5/2023 12:30 AM, Bart Van Assche wrote:
> > > > > > On 12/4/23 06:30, Maramaina Naresh wrote:
> > > > > > > + /* This capability allows the host controller driver to
> > > > > > > use the PM QoS
> > > > > > > + * feature.
> > > > > > > + */
> > > > > > > + UFSHCD_CAP_PM_QOS = 1 << 13,
> > > > > > > };
> > > > > >
> > > > > > Why does it depend on the host driver whether or not PM QoS is
> > > > > > enabled? Why isn't it enabled unconditionally?
> > > > >
> > > > > For some platform vendors power KPI might be more important than
> > > > > random io KPI. Hence this flag is disabled by default and can be
> > > > > enabled based on platform requirement.
> > > >
> > > > How about leaving this flag out unless if a host vendor asks explicitly
> > > > for this flag?
> > >
> > > IMHO, instead of completely removing this flag, how about having
> > > flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
> > > by default and if some host vendor wants to disable it explicitly,
> > > they can enable that flag.
> > > Please let me know your opinion.
>
> That would result in a flag that is tested but that is never set by
> upstream code. I'm not sure that's acceptable.
>
Agree. The flag shouldn't be introduced if there are no users.
> > If a vendor wants to disable this feature, then the driver has to be modified.
> > That won't be very convenient. So either this has to be configured through sysfs
> > or Kconfig if flexibility matters.
>
> Kconfig sounds worse to me because changing any Kconfig flag requires a
> modification of the Android GKI kernel.
>
Hmm, ok. Then I think we can have a sysfs hook to toggle the enable switch.
- Mani
> Thanks,
>
> Bart.
--
மணிவண்ணன் சதாசிவம்
On 12/7/2023 3:13 PM, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 03:02:04PM -1000, Bart Van Assche wrote:
>> On 12/6/23 05:32, Manivannan Sadhasivam wrote:
>>> On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
>>>> On 12/5/2023 10:41 PM, Bart Van Assche wrote:
>>>>> On 12/4/23 21:58, Naresh Maramaina wrote:
>>>>>> On 12/5/2023 12:30 AM, Bart Van Assche wrote:
>>>>>>> On 12/4/23 06:30, Maramaina Naresh wrote:
>>>>>>>> + /* This capability allows the host controller driver to
>>>>>>>> use the PM QoS
>>>>>>>> + * feature.
>>>>>>>> + */
>>>>>>>> + UFSHCD_CAP_PM_QOS = 1 << 13,
>>>>>>>> };
>>>>>>>
>>>>>>> Why does it depend on the host driver whether or not PM QoS is
>>>>>>> enabled? Why isn't it enabled unconditionally?
>>>>>>
>>>>>> For some platform vendors power KPI might be more important than
>>>>>> random io KPI. Hence this flag is disabled by default and can be
>>>>>> enabled based on platform requirement.
>>>>>
>>>>> How about leaving this flag out unless if a host vendor asks explicitly
>>>>> for this flag?
>>>>
>>>> IMHO, instead of completely removing this flag, how about having
>>>> flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
>>>> by default and if some host vendor wants to disable it explicitly,
>>>> they can enable that flag.
>>>> Please let me know your opinion.
>>
>> That would result in a flag that is tested but that is never set by
>> upstream code. I'm not sure that's acceptable.
>>
>
> Agree. The flag shouldn't be introduced if there are no users.
>
>>> If a vendor wants to disable this feature, then the driver has to be modified.
>>> That won't be very convenient. So either this has to be configured through sysfs
>>> or Kconfig if flexibility matters.
>>
>> Kconfig sounds worse to me because changing any Kconfig flag requires a
>> modification of the Android GKI kernel.
>>
>
> Hmm, ok. Then I think we can have a sysfs hook to toggle the enable switch.
Hi Bart, Mani
How about keeping this feature enabled by default and having a module
parameter to disable pmqos feature if required ?
Regards,
Nitin
>
> - Mani
>
>> Thanks,
>>
>> Bart.
>
On Thu, Dec 07, 2023 at 03:56:43PM +0530, Nitin Rawat wrote:
>
>
> On 12/7/2023 3:13 PM, Manivannan Sadhasivam wrote:
> > On Wed, Dec 06, 2023 at 03:02:04PM -1000, Bart Van Assche wrote:
> > > On 12/6/23 05:32, Manivannan Sadhasivam wrote:
> > > > On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
> > > > > On 12/5/2023 10:41 PM, Bart Van Assche wrote:
> > > > > > On 12/4/23 21:58, Naresh Maramaina wrote:
> > > > > > > On 12/5/2023 12:30 AM, Bart Van Assche wrote:
> > > > > > > > On 12/4/23 06:30, Maramaina Naresh wrote:
> > > > > > > > > + /* This capability allows the host controller driver to
> > > > > > > > > use the PM QoS
> > > > > > > > > + * feature.
> > > > > > > > > + */
> > > > > > > > > + UFSHCD_CAP_PM_QOS = 1 << 13,
> > > > > > > > > };
> > > > > > > >
> > > > > > > > Why does it depend on the host driver whether or not PM QoS is
> > > > > > > > enabled? Why isn't it enabled unconditionally?
> > > > > > >
> > > > > > > For some platform vendors power KPI might be more important than
> > > > > > > random io KPI. Hence this flag is disabled by default and can be
> > > > > > > enabled based on platform requirement.
> > > > > >
> > > > > > How about leaving this flag out unless if a host vendor asks explicitly
> > > > > > for this flag?
> > > > >
> > > > > IMHO, instead of completely removing this flag, how about having
> > > > > flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
> > > > > by default and if some host vendor wants to disable it explicitly,
> > > > > they can enable that flag.
> > > > > Please let me know your opinion.
> > >
> > > That would result in a flag that is tested but that is never set by
> > > upstream code. I'm not sure that's acceptable.
> > >
> >
> > Agree. The flag shouldn't be introduced if there are no users.
> >
> > > > If a vendor wants to disable this feature, then the driver has to be modified.
> > > > That won't be very convenient. So either this has to be configured through sysfs
> > > > or Kconfig if flexibility matters.
> > >
> > > Kconfig sounds worse to me because changing any Kconfig flag requires a
> > > modification of the Android GKI kernel.
> > >
> >
> > Hmm, ok. Then I think we can have a sysfs hook to toggle the enable switch.
>
> Hi Bart, Mani
>
> How about keeping this feature enabled by default and having a module
> parameter to disable pmqos feature if required ?
>
Module params not encouraged these days unless there are no other feasible
options available.
- Mani
> Regards,
> Nitin
>
> >
> > - Mani
> >
> > > Thanks,
> > >
> > > Bart.
> >
>
--
மணிவண்ணன் சதாசிவம்
On 12/6/2023 8:56 PM, Manivannan Sadhasivam wrote:
> On Mon, Dec 04, 2023 at 08:00:59PM +0530, Maramaina Naresh wrote:
>> Register ufs driver to CPU latency PM QoS framework can improves
>> ufs device random io performance.
>>
>> PM QoS initialization will insert new QoS request into the CPU
>> latency QoS list with the maximum latency PM_QOS_DEFAULT_VALUE
>> value.
>>
>> UFS driver will vote for performance mode on scale up and power
>> save mode for scale down.
>>
>> If clock scaling feature is not enabled then voting will be based
>> on clock on or off condition.
>>
>> tiotest benchmark tool io performance results on sm8550 platform:
>>
>> 1. Without PM QoS support
>> Type (Speed in) | Average of 18 iterations
>> Random Write(IPOS) | 41065.13
>> Random Read(IPOS) | 37101.3
>>
>> 2. With PM QoS support
>> Type (Speed in) | Average of 18 iterations
>> Random Write(IPOS) | 46784.9
>> Random Read(IPOS) | 42943.4
>> (Improvement % with PM QoS = ~15%).
>>
>> Co-developed-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Naveen Kumar Goud Arepalli <[email protected]>
>> Signed-off-by: Maramaina Naresh <[email protected]>
>> ---
>> drivers/ufs/core/ufshcd-priv.h | 8 +++++
>> drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++++++++++++++++
>> include/ufs/ufshcd.h | 16 +++++++++
>> 3 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>> index f42d99ce5bf1..536805f6c4e1 100644
>> --- a/drivers/ufs/core/ufshcd-priv.h
>> +++ b/drivers/ufs/core/ufshcd-priv.h
>> @@ -241,6 +241,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>> hba->vops->config_scaling_param(hba, p, data);
>> }
>>
>> +static inline u32 ufshcd_vops_config_qos_vote(struct ufs_hba *hba)
>> +{
>> + if (hba->vops && hba->vops->config_qos_vote)
>> + return hba->vops->config_qos_vote(hba);
>
> Please remove this callback as Bart noted.
>
Sure Mani, will takecare of this comment.
>> +
>> + return UFSHCD_QOS_DEFAULT_VOTE;
>> +}
>> +
>> static inline void ufshcd_vops_reinit_notify(struct ufs_hba *hba)
>> {
>> if (hba->vops && hba->vops->reinit_notify)
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index ae9936fc6ffb..13370febd2b5 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1001,6 +1001,20 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>> return ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6;
>> }
>>
>> +/**
>> + * ufshcd_pm_qos_perf - vote for PM QoS performance or power save mode
>
> ufshcd_pm_qos_update() - Update PM QoS request
>
Sure Mani, will takecare of this comment.
>> + * @hba: per adapter instance
>> + * @on: If True, vote for perf PM QoS mode otherwise power save mode
>> + */
>> +static void ufshcd_pm_qos_perf(struct ufs_hba *hba, bool on)
>> +{
>> + if (!hba->pm_qos_init)
>> + return;
>> +
>> + cpu_latency_qos_update_request(&hba->pm_qos_req, on ? hba->qos_vote
>> + : PM_QOS_DEFAULT_VALUE);
>> +}
>> +
>> /**
>> * ufshcd_set_clk_freq - set UFS controller clock frequencies
>> * @hba: per adapter instance
>> @@ -1153,6 +1167,10 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>> trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
>> (scale_up ? "up" : "down"),
>> ktime_to_us(ktime_sub(ktime_get(), start)), ret);
>> +
>> + if (!ret)
>> + ufshcd_pm_qos_perf(hba, scale_up);
>
> Can't you just move this before trace_ufshcd_profile_clk_scaling()? This also
> avoids checking for !ret.
>
In this case, we need to use goto out; inside if(ret) of
ufshcd_vops_clk_scale_notify.
will do the above change, to enable ufshcd_pm_qos_perf before the out flag.
>> +
>> return ret;
>> }
>>
>> @@ -9204,6 +9222,8 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
>> if (ret)
>> return ret;
>>
>> + if (!ufshcd_is_clkscaling_supported(hba))
>> + ufshcd_pm_qos_perf(hba, on);
>> out:
>> if (ret) {
>> list_for_each_entry(clki, head, list) {
>> @@ -9296,6 +9316,45 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
>> return ret;
>> }
>>
>> +/**
>> + * ufshcd_pm_qos_init - initialize PM QoS instance
>
> "Initialize PM QoS request"
>
Sure Mani, will takecare of this comment.
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_pm_qos_init(struct ufs_hba *hba)
>> +{
>> + if (!(hba->caps & UFSHCD_CAP_PM_QOS))
>> + return;
>> +
>> + /*
>> + * called to configure PM QoS vote value for UFS host,
>> + * expecting qos vote return value from caller else
>> + * default vote value will be return.
>> + */
>> + hba->qos_vote = ufshcd_vops_config_qos_vote(hba);
>
> No need of this variable too if you get rid of the callback.
>
>> + cpu_latency_qos_add_request(&hba->pm_qos_req,
>> + PM_QOS_DEFAULT_VALUE);
>> +
>> + if (cpu_latency_qos_request_active(&hba->pm_qos_req))
>> + hba->pm_qos_init = true;
>
> Why do you need this flag?
this flag ensure UFS qos request got added into the Global PM QoS list.
>
>> +
>> + dev_dbg(hba->dev, "%s: QoS %s, qos_vote: %u\n", __func__,
>> + hba->pm_qos_init ? "initialized" : "uninitialized",
>> + hba->qos_vote);
>> +}
>> +
>> +/**
>> + * ufshcd_pm_qos_exit - remove instance from PM QoS
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>> +{
>> + if (!hba->pm_qos_init)
>> + return;
>> +
>> + cpu_latency_qos_remove_request(&hba->pm_qos_req);
>> + hba->pm_qos_init = false;
>> +}
>> +
>
> [...]
>
>> /**
>> * struct ufs_hba - per adapter private structure
>> * @mmio_base: UFSHCI base register address
>> @@ -912,6 +923,8 @@ enum ufshcd_mcq_opr {
>> * @mcq_base: Multi circular queue registers base address
>> * @uhq: array of supported hardware queues
>> * @dev_cmd_queue: Queue for issuing device management commands
>> + * @pm_qos_req: PM QoS request handle
>> + * @pm_qos_init: flag to check if pm qos init completed
>> */
>> struct ufs_hba {
>> void __iomem *mmio_base;
>> @@ -1076,6 +1089,9 @@ struct ufs_hba {
>> struct ufs_hw_queue *uhq;
>> struct ufs_hw_queue *dev_cmd_queue;
>> struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
>> + struct pm_qos_request pm_qos_req;
>> + bool pm_qos_init;
>> + u32 qos_vote;
>
> Order doesn't match Kdoc.
>
we are removing qos_vote variable in next patch series.
> - Mani
>
Thanks,
Naresh.
On 12/6/2023 8:56 PM, Manivannan Sadhasivam wrote:
> On Mon, Dec 04, 2023 at 08:00:59PM +0530, Maramaina Naresh wrote:
>> Register ufs driver to CPU latency PM QoS framework can improves
>> ufs device random io performance.
>>
>> PM QoS initialization will insert new QoS request into the CPU
>> latency QoS list with the maximum latency PM_QOS_DEFAULT_VALUE
>> value.
>>
>> UFS driver will vote for performance mode on scale up and power
>> save mode for scale down.
>>
>> If clock scaling feature is not enabled then voting will be based
>> on clock on or off condition.
>>
>> tiotest benchmark tool io performance results on sm8550 platform:
>>
>> 1. Without PM QoS support
>> Type (Speed in) | Average of 18 iterations
>> Random Write(IPOS) | 41065.13
>> Random Read(IPOS) | 37101.3
>>
>> 2. With PM QoS support
>> Type (Speed in) | Average of 18 iterations
>> Random Write(IPOS) | 46784.9
>> Random Read(IPOS) | 42943.4
>> (Improvement % with PM QoS = ~15%).
>>
>> Co-developed-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Naveen Kumar Goud Arepalli <[email protected]>
>> Signed-off-by: Maramaina Naresh <[email protected]>
>> ---
>> drivers/ufs/core/ufshcd-priv.h | 8 +++++
>> drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++++++++++++++++
>> include/ufs/ufshcd.h | 16 +++++++++
>> 3 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>> index f42d99ce5bf1..536805f6c4e1 100644
>> --- a/drivers/ufs/core/ufshcd-priv.h
>> +++ b/drivers/ufs/core/ufshcd-priv.h
>> @@ -241,6 +241,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>> hba->vops->config_scaling_param(hba, p, data);
>> }
>>
>> +static inline u32 ufshcd_vops_config_qos_vote(struct ufs_hba *hba)
>> +{
>> + if (hba->vops && hba->vops->config_qos_vote)
>> + return hba->vops->config_qos_vote(hba);
>
> Please remove this callback as Bart noted.
>
Sure Mani, will takecare of this comment.
>> +
>> + return UFSHCD_QOS_DEFAULT_VOTE;
>> +}
>> +
>> static inline void ufshcd_vops_reinit_notify(struct ufs_hba *hba)
>> {
>> if (hba->vops && hba->vops->reinit_notify)
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index ae9936fc6ffb..13370febd2b5 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1001,6 +1001,20 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>> return ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6;
>> }
>>
>> +/**
>> + * ufshcd_pm_qos_perf - vote for PM QoS performance or power save mode
>
> ufshcd_pm_qos_update() - Update PM QoS request
>
Sure Mani, will takecare of this comment.
>> + * @hba: per adapter instance
>> + * @on: If True, vote for perf PM QoS mode otherwise power save mode
>> + */
>> +static void ufshcd_pm_qos_perf(struct ufs_hba *hba, bool on)
>> +{
>> + if (!hba->pm_qos_init)
>> + return;
>> +
>> + cpu_latency_qos_update_request(&hba->pm_qos_req, on ? hba->qos_vote
>> + : PM_QOS_DEFAULT_VALUE);
>> +}
>> +
>> /**
>> * ufshcd_set_clk_freq - set UFS controller clock frequencies
>> * @hba: per adapter instance
>> @@ -1153,6 +1167,10 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>> trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
>> (scale_up ? "up" : "down"),
>> ktime_to_us(ktime_sub(ktime_get(), start)), ret);
>> +
>> + if (!ret)
>> + ufshcd_pm_qos_perf(hba, scale_up);
>
> Can't you just move this before trace_ufshcd_profile_clk_scaling()? This also
> avoids checking for !ret.
>
In this case, we need to use goto out; inside if condition of
ufshcd_vops_clk_scale_notify.
we can enable ufshcd_pm_qos_perf only when ufshcd_vops_clk_scale_notify
is successful.
Will add goto out; in next patch set.
>> +
>> return ret;
>> }
>>
>> @@ -9204,6 +9222,8 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on)
>> if (ret)
>> return ret;
>>
>> + if (!ufshcd_is_clkscaling_supported(hba))
>> + ufshcd_pm_qos_perf(hba, on);
>> out:
>> if (ret) {
>> list_for_each_entry(clki, head, list) {
>> @@ -9296,6 +9316,45 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
>> return ret;
>> }
>>
>> +/**
>> + * ufshcd_pm_qos_init - initialize PM QoS instance
>
> "Initialize PM QoS request"
>
Sure Mani, will takecare of this comment.
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_pm_qos_init(struct ufs_hba *hba)
>> +{
>> + if (!(hba->caps & UFSHCD_CAP_PM_QOS))
>> + return;
>> +
>> + /*
>> + * called to configure PM QoS vote value for UFS host,
>> + * expecting qos vote return value from caller else
>> + * default vote value will be return.
>> + */
>> + hba->qos_vote = ufshcd_vops_config_qos_vote(hba);
>
> No need of this variable too if you get rid of the callback.
>
>> + cpu_latency_qos_add_request(&hba->pm_qos_req,
>> + PM_QOS_DEFAULT_VALUE);
>> +
>> + if (cpu_latency_qos_request_active(&hba->pm_qos_req))
>> + hba->pm_qos_init = true;
>
> Why do you need this flag?
this flag ensure UFS qos request got added into the list.
>
>> +
>> + dev_dbg(hba->dev, "%s: QoS %s, qos_vote: %u\n", __func__,
>> + hba->pm_qos_init ? "initialized" : "uninitialized",
>> + hba->qos_vote);
>> +}
>> +
>> +/**
>> + * ufshcd_pm_qos_exit - remove instance from PM QoS
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>> +{
>> + if (!hba->pm_qos_init)
>> + return;
>> +
>> + cpu_latency_qos_remove_request(&hba->pm_qos_req);
>> + hba->pm_qos_init = false;
>> +}
>> +
>
> [...]
>
>> /**
>> * struct ufs_hba - per adapter private structure
>> * @mmio_base: UFSHCI base register address
>> @@ -912,6 +923,8 @@ enum ufshcd_mcq_opr {
>> * @mcq_base: Multi circular queue registers base address
>> * @uhq: array of supported hardware queues
>> * @dev_cmd_queue: Queue for issuing device management commands
>> + * @pm_qos_req: PM QoS request handle
>> + * @pm_qos_init: flag to check if pm qos init completed
>> */
>> struct ufs_hba {
>> void __iomem *mmio_base;
>> @@ -1076,6 +1089,9 @@ struct ufs_hba {
>> struct ufs_hw_queue *uhq;
>> struct ufs_hw_queue *dev_cmd_queue;
>> struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
>> + struct pm_qos_request pm_qos_req;
>> + bool pm_qos_init;
>> + u32 qos_vote;
>
> Order doesn't match Kdoc.
>
qos_vote variable will be remove as per latest comment.
> - Mani
>
Thanks,
Naresh.
On Thu, Dec 07, 2023 at 04:51:01PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Dec 07, 2023 at 03:56:43PM +0530, Nitin Rawat wrote:
> >
> >
> > On 12/7/2023 3:13 PM, Manivannan Sadhasivam wrote:
> > > On Wed, Dec 06, 2023 at 03:02:04PM -1000, Bart Van Assche wrote:
> > > > On 12/6/23 05:32, Manivannan Sadhasivam wrote:
> > > > > On Wed, Dec 06, 2023 at 07:32:54PM +0530, Naresh Maramaina wrote:
> > > > > > On 12/5/2023 10:41 PM, Bart Van Assche wrote:
> > > > > > > On 12/4/23 21:58, Naresh Maramaina wrote:
> > > > > > > > On 12/5/2023 12:30 AM, Bart Van Assche wrote:
> > > > > > > > > On 12/4/23 06:30, Maramaina Naresh wrote:
> > > > > > > > > > +??? /* This capability allows the host controller driver to
> > > > > > > > > > use the PM QoS
> > > > > > > > > > +???? * feature.
> > > > > > > > > > +???? */
> > > > > > > > > > +??? UFSHCD_CAP_PM_QOS??????????????? = 1 << 13,
> > > > > > > > > > ? };
> > > > > > > > >
> > > > > > > > > Why does it depend on the host driver whether or not PM QoS is
> > > > > > > > > enabled? Why isn't it enabled unconditionally?
> > > > > > > >
> > > > > > > > For some platform vendors power KPI might be more important than
> > > > > > > > random io KPI. Hence this flag is disabled by default and can be
> > > > > > > > enabled based on platform requirement.
> > > > > > >
> > > > > > > How about leaving this flag out unless if a host vendor asks explicitly
> > > > > > > for this flag?
> > > > > >
> > > > > > IMHO, instead of completely removing this flag, how about having
> > > > > > flag like "UFSHCD_CAP_DISABLE_PM_QOS" which will make PMQOS enable
> > > > > > by default and if some host vendor wants to disable it explicitly,
> > > > > > they can enable that flag.
> > > > > > Please let me know your opinion.
> > > >
> > > > That would result in a flag that is tested but that is never set by
> > > > upstream code. I'm not sure that's acceptable.
> > > >
> > >
> > > Agree. The flag shouldn't be introduced if there are no users.
> > >
> > > > > If a vendor wants to disable this feature, then the driver has to be modified.
> > > > > That won't be very convenient. So either this has to be configured through sysfs
> > > > > or Kconfig if flexibility matters.
> > > >
> > > > Kconfig sounds worse to me because changing any Kconfig flag requires a
> > > > modification of the Android GKI kernel.
> > > >
> > >
> > > Hmm, ok. Then I think we can have a sysfs hook to toggle the enable switch.
> >
> > Hi Bart, Mani
> >
> > How about keeping this feature enabled by default and having a module
> > parameter to disable pmqos feature if required ?
> >
>
> Module params not encouraged these days unless there are no other feasible
> options available.
Yeah, one of the problem with module param is that we can't have
different settings for two two UFS controllers. Anyways, this setting
need not be a module param, there is nothing special about this setting
that is tied to module loading (driver probe) time, AFAICT.
Thanks,
Pavan