Hi,
This series adds OPP (Operating Points) support to UFSHCD driver and
interconnect support to Qcom UFS driver.
Motivation behind adding OPP support is to scale both clocks as well as
regulators/performance state dynamically. Currently, UFSHCD just scales
clock frequency during runtime with the help of "freq-table-hz" property
defined in devicetree. With the addition of OPP tables in devicetree (as
done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
both clocks and performance state of power domain which helps in power
saving.
For the addition of OPP support to UFSHCD, there are changes required to
the OPP framework and devfreq drivers which are also added in this series.
Finally, interconnect support is added to Qcom UFS driver for scaling the
interconnect path dynamically. This is required to avoid boot crash in
recent SoCs and also to save power during runtime. More information is
available in patch 13/13.
Credits
=======
This series is a continuation of previous work by Krzysztof Kozlowski [1]
and Brian Masney [2]. Ideally, this could've split into two series (OPP
and interconnect) but since there will be a dependency in the devicetree,
I decided to keep them in a single series.
Testing
=======
This series is tested on 96Boards RB3 (SDM845 SoC) and RB5 (SM8250 SoC)
development boards.
Merging Strategy
================
An immutable branch might be required between OPP and SCSI trees because of
the API dependency (devfreq too). And I leave it up to the maintainers to
decide.
Thanks,
Mani
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Changes in v2:
* Added more description to the bindings patch 2/15
* Fixed dev_pm_opp_put() usage in patch 10/15
* Added a new patch for adding enums for UFS lanes 14/15
* Changed the icc variables to mem_bw and cfg_bw and used
the enums for gears and lanes in bw_table
* Collected review tags
* Added SCSI list and folks
* Removed duplicate patches
Krzysztof Kozlowski (2):
dt-bindings: ufs: common: add OPP table
arm64: dts: qcom: sdm845: Add OPP table support to UFSHC
Manivannan Sadhasivam (13):
dt-bindings: opp: Increase maxItems for opp-hz property
arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
arm64: dts: qcom: sm8250: Add OPP table support to UFSHC
OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs
OPP: Introduce dev_pm_opp_get_freq_indexed() API
PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed()
APIs
scsi: ufs: core: Add OPP support for scaling clocks and regulators
scsi: ufs: host: Add support for parsing OPP
arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
scsi: ufs: core: Add enums for UFS lanes
scsi: ufs: qcom: Add support for scaling interconnects
.../devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
.../devicetree/bindings/ufs/ufs-common.yaml | 34 +++-
arch/arm64/boot/dts/qcom/sdm845.dtsi | 47 ++++--
arch/arm64/boot/dts/qcom/sm8250.dtsi | 43 +++--
drivers/devfreq/devfreq.c | 14 +-
drivers/opp/core.c | 76 +++++++++
drivers/ufs/core/ufshcd.c | 148 +++++++++++++-----
drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++-
drivers/ufs/host/ufs-qcom.h | 3 +
drivers/ufs/host/ufshcd-pltfrm.c | 120 +++++++++++++-
include/linux/pm_opp.h | 26 +++
include/ufs/ufshcd.h | 4 +
include/ufs/unipro.h | 6 +
13 files changed, 586 insertions(+), 68 deletions(-)
--
2.25.1
UFS core is only scaling the clocks during devfreq scaling and
initialization. But for an optimum power saving, regulators should also
be scaled along with the clocks.
So let's use the OPP framework which supports scaling clocks, regulators,
and performance state using OPP table defined in devicetree. For
accomodating the OPP support, the existing APIs (ufshcd_scale_clks,
ufshcd_is_devfreq_scaling_required and ufshcd_devfreq_scale) are modified
to accept "freq" as an argument which in turn used by the OPP helpers.
The OPP support is added along with the old freq-table based clock scaling
so that the existing platforms work as expected.
Co-developed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/core/ufshcd.c | 144 +++++++++++++++++++++++++++++---------
include/ufs/ufshcd.h | 4 ++
2 files changed, 115 insertions(+), 33 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 983fae84d9e8..6207afac729d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -20,6 +20,7 @@
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/module.h>
+#include <linux/pm_opp.h>
#include <linux/regulator/consumer.h>
#include <linux/sched/clock.h>
#include <scsi/scsi_cmnd.h>
@@ -276,7 +277,8 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
static void ufshcd_resume_clkscaling(struct ufs_hba *hba);
static void ufshcd_suspend_clkscaling(struct ufs_hba *hba);
static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
+static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
+ bool scale_up);
static irqreturn_t ufshcd_intr(int irq, void *__hba);
static int ufshcd_change_power_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *pwr_mode);
@@ -1087,15 +1089,33 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
return ret;
}
+static int ufshcd_opp_set_rate(struct ufs_hba *hba, unsigned long freq)
+{
+ struct dev_pm_opp *opp;
+ int ret;
+
+ opp = dev_pm_opp_find_freq_floor_indexed(hba->dev,
+ &freq, 0);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = dev_pm_opp_set_opp(hba->dev, opp);
+ dev_pm_opp_put(opp);
+
+ return ret;
+}
+
/**
* ufshcd_scale_clks - scale up or scale down UFS controller clocks
* @hba: per adapter instance
+ * @freq: frequency to scale
* @scale_up: True if scaling up and false if scaling down
*
* Returns 0 if successful
* Returns < 0 for any other errors
*/
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
+ bool scale_up)
{
int ret = 0;
ktime_t start = ktime_get();
@@ -1104,13 +1124,21 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
if (ret)
goto out;
- ret = ufshcd_set_clk_freq(hba, scale_up);
+ if (hba->use_pm_opp)
+ ret = ufshcd_opp_set_rate(hba, freq);
+ else
+ ret = ufshcd_set_clk_freq(hba, scale_up);
if (ret)
goto out;
ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
- if (ret)
- ufshcd_set_clk_freq(hba, !scale_up);
+ if (ret) {
+ if (hba->use_pm_opp)
+ ufshcd_opp_set_rate(hba,
+ hba->devfreq->previous_freq);
+ else
+ ufshcd_set_clk_freq(hba, !scale_up);
+ }
out:
trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
@@ -1122,12 +1150,13 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
/**
* ufshcd_is_devfreq_scaling_required - check if scaling is required or not
* @hba: per adapter instance
+ * @freq: frequency to scale
* @scale_up: True if scaling up and false if scaling down
*
* Returns true if scaling is required, false otherwise.
*/
static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
- bool scale_up)
+ unsigned long freq, bool scale_up)
{
struct ufs_clk_info *clki;
struct list_head *head = &hba->clk_list_head;
@@ -1135,6 +1164,9 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
if (list_empty(head))
return false;
+ if (hba->use_pm_opp)
+ return freq != hba->clk_scaling.target_freq;
+
list_for_each_entry(clki, head, list) {
if (!IS_ERR_OR_NULL(clki->clk)) {
if (scale_up && clki->max_freq) {
@@ -1331,13 +1363,15 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
/**
* ufshcd_devfreq_scale - scale up/down UFS clocks and gear
* @hba: per adapter instance
+ * @freq: frequency to scale
* @scale_up: True for scaling up and false for scalin down
*
* Returns 0 for success,
* Returns -EBUSY if scaling can't happen at this time
* Returns non-zero for any other errors
*/
-static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
+ bool scale_up)
{
int ret = 0;
@@ -1352,7 +1386,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
goto out_unprepare;
}
- ret = ufshcd_scale_clks(hba, scale_up);
+ ret = ufshcd_scale_clks(hba, freq, scale_up);
if (ret) {
if (!scale_up)
ufshcd_scale_gear(hba, true);
@@ -1363,7 +1397,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
if (scale_up) {
ret = ufshcd_scale_gear(hba, true);
if (ret) {
- ufshcd_scale_clks(hba, false);
+ ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
+ false);
goto out_unprepare;
}
}
@@ -1421,9 +1456,22 @@ static int ufshcd_devfreq_target(struct device *dev,
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
- clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
- /* Override with the closest supported frequency */
- *freq = (unsigned long) clk_round_rate(clki->clk, *freq);
+ if (hba->use_pm_opp) {
+ struct dev_pm_opp *opp;
+
+ /* Get the recommended frequency from OPP framework */
+ opp = devfreq_recommended_opp(dev, freq, flags);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ dev_pm_opp_put(opp);
+ } else {
+ /* Override with the closest supported frequency */
+ clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info,
+ list);
+ *freq = (unsigned long) clk_round_rate(clki->clk, *freq);
+ }
+
spin_lock_irqsave(hba->host->host_lock, irq_flags);
if (ufshcd_eh_in_progress(hba)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1438,12 +1486,17 @@ static int ufshcd_devfreq_target(struct device *dev,
goto out;
}
- /* Decide based on the rounded-off frequency and update */
- scale_up = *freq == clki->max_freq;
- if (!scale_up)
+ /* Decide based on the target or rounded-off frequency and update */
+ if (hba->use_pm_opp)
+ scale_up = *freq > hba->clk_scaling.target_freq;
+ else
+ scale_up = *freq == clki->max_freq;
+
+ if (!hba->use_pm_opp && !scale_up)
*freq = clki->min_freq;
+
/* Update the frequency */
- if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
+ if (!ufshcd_is_devfreq_scaling_required(hba, *freq, scale_up)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
ret = 0;
goto out; /* no state change required */
@@ -1451,7 +1504,9 @@ static int ufshcd_devfreq_target(struct device *dev,
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
start = ktime_get();
- ret = ufshcd_devfreq_scale(hba, scale_up);
+ ret = ufshcd_devfreq_scale(hba, *freq, scale_up);
+ if (!ret)
+ hba->clk_scaling.target_freq = *freq;
trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
(scale_up ? "up" : "down"),
@@ -1471,8 +1526,6 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
struct ufs_hba *hba = dev_get_drvdata(dev);
struct ufs_clk_scaling *scaling = &hba->clk_scaling;
unsigned long flags;
- struct list_head *clk_list = &hba->clk_list_head;
- struct ufs_clk_info *clki;
ktime_t curr_t;
if (!ufshcd_is_clkscaling_supported(hba))
@@ -1485,17 +1538,24 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
if (!scaling->window_start_t)
goto start_window;
- clki = list_first_entry(clk_list, struct ufs_clk_info, list);
/*
* If current frequency is 0, then the ondemand governor considers
* there's no initial frequency set. And it always requests to set
* to max. frequency.
*/
- stat->current_frequency = clki->curr_freq;
+ if (hba->use_pm_opp) {
+ stat->current_frequency = hba->clk_scaling.target_freq;
+ } else {
+ struct list_head *clk_list = &hba->clk_list_head;
+ struct ufs_clk_info *clki;
+
+ clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+ stat->current_frequency = clki->curr_freq;
+ }
+
if (scaling->is_busy_started)
scaling->tot_busy_t += ktime_us_delta(curr_t,
scaling->busy_start_t);
-
stat->total_time = ktime_us_delta(curr_t, scaling->window_start_t);
stat->busy_time = scaling->tot_busy_t;
start_window:
@@ -1524,9 +1584,11 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
if (list_empty(clk_list))
return 0;
- clki = list_first_entry(clk_list, struct ufs_clk_info, list);
- dev_pm_opp_add(hba->dev, clki->min_freq, 0);
- dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+ if (!hba->use_pm_opp) {
+ clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+ dev_pm_opp_add(hba->dev, clki->min_freq, 0);
+ dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+ }
ufshcd_vops_config_scaling_param(hba, &hba->vps->devfreq_profile,
&hba->vps->ondemand_data);
@@ -1538,8 +1600,10 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
ret = PTR_ERR(devfreq);
dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
- dev_pm_opp_remove(hba->dev, clki->min_freq);
- dev_pm_opp_remove(hba->dev, clki->max_freq);
+ if (!hba->use_pm_opp) {
+ dev_pm_opp_remove(hba->dev, clki->min_freq);
+ dev_pm_opp_remove(hba->dev, clki->max_freq);
+ }
return ret;
}
@@ -1551,7 +1615,6 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
static void ufshcd_devfreq_remove(struct ufs_hba *hba)
{
struct list_head *clk_list = &hba->clk_list_head;
- struct ufs_clk_info *clki;
if (!hba->devfreq)
return;
@@ -1559,9 +1622,13 @@ static void ufshcd_devfreq_remove(struct ufs_hba *hba)
devfreq_remove_device(hba->devfreq);
hba->devfreq = NULL;
- clki = list_first_entry(clk_list, struct ufs_clk_info, list);
- dev_pm_opp_remove(hba->dev, clki->min_freq);
- dev_pm_opp_remove(hba->dev, clki->max_freq);
+ if (!hba->use_pm_opp) {
+ struct ufs_clk_info *clki;
+
+ clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+ dev_pm_opp_remove(hba->dev, clki->min_freq);
+ dev_pm_opp_remove(hba->dev, clki->max_freq);
+ }
}
static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
@@ -1646,7 +1713,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
ufshcd_resume_clkscaling(hba);
} else {
ufshcd_suspend_clkscaling(hba);
- err = ufshcd_devfreq_scale(hba, true);
+ err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
if (err)
dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
__func__, err);
@@ -7666,7 +7733,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
hba->silence_err_logs = false;
/* scale up clocks to max frequency before full reinitialization */
- ufshcd_scale_clks(hba, true);
+ ufshcd_scale_clks(hba, ULONG_MAX, true);
err = ufshcd_hba_enable(hba);
@@ -9185,6 +9252,17 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
dev_dbg(dev, "%s: clk: %s, rate: %lu\n", __func__,
clki->name, clk_get_rate(clki->clk));
}
+
+ /* Set Max. frequency for all clocks */
+ if (hba->use_pm_opp) {
+ ret = ufshcd_opp_set_rate(hba, ULONG_MAX);
+ if (ret) {
+ dev_err(hba->dev, "%s: failed to set OPP: %d", __func__,
+ ret);
+ goto out;
+ }
+ }
+
out:
return ret;
}
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 6dc11fa0ebb1..9f61b6d56d11 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -407,6 +407,7 @@ struct ufs_clk_gating {
* @workq: workqueue to schedule devfreq suspend/resume work
* @suspend_work: worker to suspend devfreq
* @resume_work: worker to resume devfreq
+ * @target_freq: frequency requested by devfreq framework
* @min_gear: lowest HS gear to scale down to
* @is_enabled: tracks if scaling is currently enabled or not, controlled by
* clkscale_enable sysfs node
@@ -426,6 +427,7 @@ struct ufs_clk_scaling {
struct workqueue_struct *workq;
struct work_struct suspend_work;
struct work_struct resume_work;
+ unsigned long target_freq;
u32 min_gear;
bool is_enabled;
bool is_allowed;
@@ -870,6 +872,7 @@ enum ufshcd_mcq_opr {
* @auto_bkops_enabled: to track whether bkops is enabled in device
* @vreg_info: UFS device voltage regulator information
* @clk_list_head: UFS host controller clocks list node head
+ * @use_pm_opp: Indicates whether OPP based scaling is used or not
* @req_abort_count: number of times ufshcd_abort() has been called
* @lanes_per_direction: number of lanes per data direction between the UFS
* controller and the UFS device.
@@ -1021,6 +1024,7 @@ struct ufs_hba {
bool auto_bkops_enabled;
struct ufs_vreg_info vreg_info;
struct list_head clk_list_head;
+ bool use_pm_opp;
/* Number of requests aborts */
int req_abort_count;
--
2.25.1
Since there are enums available for UFS gears, let's add enums for lanes
as well to maintain uniformity.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/core/ufshcd.c | 4 ++--
drivers/ufs/host/ufshcd-pltfrm.c | 4 ++--
include/ufs/unipro.h | 6 ++++++
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 6207afac729d..4496a23eaa83 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4447,8 +4447,8 @@ static void ufshcd_init_pwr_info(struct ufs_hba *hba)
{
hba->pwr_info.gear_rx = UFS_PWM_G1;
hba->pwr_info.gear_tx = UFS_PWM_G1;
- hba->pwr_info.lane_rx = 1;
- hba->pwr_info.lane_tx = 1;
+ hba->pwr_info.lane_rx = UFS_LANE_1;
+ hba->pwr_info.lane_tx = UFS_LANE_1;
hba->pwr_info.pwr_rx = SLOWAUTO_MODE;
hba->pwr_info.pwr_tx = SLOWAUTO_MODE;
hba->pwr_info.hs_rate = 0;
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index 068c22378c88..8d20cbb552aa 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -415,8 +415,8 @@ EXPORT_SYMBOL_GPL(ufshcd_get_pwr_dev_param);
void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param)
{
*dev_param = (struct ufs_dev_params){
- .tx_lanes = 2,
- .rx_lanes = 2,
+ .tx_lanes = UFS_LANE_2,
+ .rx_lanes = UFS_LANE_2,
.hs_rx_gear = UFS_HS_G3,
.hs_tx_gear = UFS_HS_G3,
.pwm_rx_gear = UFS_PWM_G4,
diff --git a/include/ufs/unipro.h b/include/ufs/unipro.h
index dc9dd1d23f0f..256eb3a43f54 100644
--- a/include/ufs/unipro.h
+++ b/include/ufs/unipro.h
@@ -230,6 +230,12 @@ enum ufs_hs_gear_tag {
UFS_HS_G5 /* HS Gear 5 */
};
+enum ufs_lanes {
+ UFS_LANE_DONT_CHANGE, /* Don't change Lane */
+ UFS_LANE_1, /* Lane 1 (default for reset) */
+ UFS_LANE_2, /* Lane 2 */
+};
+
enum ufs_unipro_ver {
UFS_UNIPRO_VER_RESERVED = 0,
UFS_UNIPRO_VER_1_40 = 1, /* UniPro version 1.40 */
--
2.25.1
On 7/19/23 22:40, Manivannan Sadhasivam wrote:
> This series adds OPP (Operating Points) support to UFSHCD driver and
> interconnect support to Qcom UFS driver.
>
> Motivation behind adding OPP support is to scale both clocks as well as
> regulators/performance state dynamically. Currently, UFSHCD just scales
> clock frequency during runtime with the help of "freq-table-hz" property
> defined in devicetree. With the addition of OPP tables in devicetree (as
> done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> both clocks and performance state of power domain which helps in power
> saving.
>
> For the addition of OPP support to UFSHCD, there are changes required to
> the OPP framework and devfreq drivers which are also added in this series.
>
> Finally, interconnect support is added to Qcom UFS driver for scaling the
> interconnect path dynamically. This is required to avoid boot crash in
> recent SoCs and also to save power during runtime. More information is
> available in patch 13/13.
How much power can OPP save? I'm asking this since I'm wondering whether
the power saved by OPP outweighs the complexity added by this patch series.
Thanks,
Bart.
On Thu, Jul 20, 2023 at 09:44:38AM -0700, Bart Van Assche wrote:
> On 7/19/23 22:40, Manivannan Sadhasivam wrote:
> > This series adds OPP (Operating Points) support to UFSHCD driver and
> > interconnect support to Qcom UFS driver.
> >
> > Motivation behind adding OPP support is to scale both clocks as well as
> > regulators/performance state dynamically. Currently, UFSHCD just scales
> > clock frequency during runtime with the help of "freq-table-hz" property
> > defined in devicetree. With the addition of OPP tables in devicetree (as
> > done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> > both clocks and performance state of power domain which helps in power
> > saving.
> >
> > For the addition of OPP support to UFSHCD, there are changes required to
> > the OPP framework and devfreq drivers which are also added in this series.
> >
> > Finally, interconnect support is added to Qcom UFS driver for scaling the
> > interconnect path dynamically. This is required to avoid boot crash in
> > recent SoCs and also to save power during runtime. More information is
> > available in patch 13/13.
>
> How much power can OPP save? I'm asking this since I'm wondering whether
> the power saved by OPP outweighs the complexity added by this patch series.
>
I haven't had a chance to do proper power measurements with this series due to
lack of access to tools. But it won't be optimal to run the clocks at high/low
frequencies without changing the associated regulator/power domain state.
Atleast on Qcom platforms, the clock frequencies are tied to RPMh (power
management entity) performance states for the peripherals. So both have to go
hand in hand. Till now, only UFS among the other peripherals is not doing it
right and hence this series.
- Mani
> Thanks,
>
> Bart.
--
மணிவண்ணன் சதாசிவம்
On 20-07-23, 11:10, Manivannan Sadhasivam wrote:
> Hi,
>
> This series adds OPP (Operating Points) support to UFSHCD driver and
> interconnect support to Qcom UFS driver.
>
> Motivation behind adding OPP support is to scale both clocks as well as
> regulators/performance state dynamically. Currently, UFSHCD just scales
> clock frequency during runtime with the help of "freq-table-hz" property
> defined in devicetree. With the addition of OPP tables in devicetree (as
> done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> both clocks and performance state of power domain which helps in power
> saving.
>
> For the addition of OPP support to UFSHCD, there are changes required to
> the OPP framework and devfreq drivers which are also added in this series.
>
> Finally, interconnect support is added to Qcom UFS driver for scaling the
> interconnect path dynamically. This is required to avoid boot crash in
> recent SoCs and also to save power during runtime. More information is
> available in patch 13/13.
Hi Mani,
I have picked the OPP related patches from here (apart from DT one)
and sent them separately in a series, along with few changes from me.
Also pushed them in my linux-next branch.
Thanks.
--
viresh
On Fri, Jul 21, 2023 at 03:12:06PM +0530, Viresh Kumar wrote:
> On 20-07-23, 11:10, Manivannan Sadhasivam wrote:
> > Hi,
> >
> > This series adds OPP (Operating Points) support to UFSHCD driver and
> > interconnect support to Qcom UFS driver.
> >
> > Motivation behind adding OPP support is to scale both clocks as well as
> > regulators/performance state dynamically. Currently, UFSHCD just scales
> > clock frequency during runtime with the help of "freq-table-hz" property
> > defined in devicetree. With the addition of OPP tables in devicetree (as
> > done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> > both clocks and performance state of power domain which helps in power
> > saving.
> >
> > For the addition of OPP support to UFSHCD, there are changes required to
> > the OPP framework and devfreq drivers which are also added in this series.
> >
> > Finally, interconnect support is added to Qcom UFS driver for scaling the
> > interconnect path dynamically. This is required to avoid boot crash in
> > recent SoCs and also to save power during runtime. More information is
> > available in patch 13/13.
>
> Hi Mani,
>
> I have picked the OPP related patches from here (apart from DT one)
> and sent them separately in a series, along with few changes from me.
> Also pushed them in my linux-next branch.
>
Thanks Viresh! For patch 8/15, Kbuild bot has identified one potential null ptr
dereference issue. Could you please fix that in your branch?
You just need to remove the opp dereference in dev_pm_opp_get_freq_indexed()
before the IS_ERR_OR_NULL() check as below:
```
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 66dc0d0cfaed..683e6e61f80b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -208,9 +208,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
*/
unsigned long dev_pm_opp_get_freq_indexed(struct dev_pm_opp *opp, u32 index)
{
- struct opp_table *opp_table = opp->opp_table;
-
- if (IS_ERR_OR_NULL(opp) || index >= opp_table->clk_count) {
+ if (IS_ERR_OR_NULL(opp) || index >= opp->opp_table->clk_count) {
pr_err("%s: Invalid parameters\n", __func__);
return 0;
}
```
- Mani
> Thanks.
>
> --
> viresh
--
மணிவண்ணன் சதாசிவம்
On Thu, 20 Jul 2023 11:10:45 +0530, Manivannan Sadhasivam wrote:
> This series adds OPP (Operating Points) support to UFSHCD driver and
> interconnect support to Qcom UFS driver.
>
> Motivation behind adding OPP support is to scale both clocks as well as
> regulators/performance state dynamically. Currently, UFSHCD just scales
> clock frequency during runtime with the help of "freq-table-hz" property
> defined in devicetree. With the addition of OPP tables in devicetree (as
> done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> both clocks and performance state of power domain which helps in power
> saving.
>
> [...]
Applied, thanks!
[03/15] arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
commit: 4b6ea15c0a1122422b44bf6c47a3c22fc8d46777
[04/15] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
commit: bbbef6e24bc4493602df68b052f6f48d48e3184a
[12/15] arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
commit: 84e2e371f4f911337604e8ba9281e950230d1189
[13/15] arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
commit: aeea56072cc8cb0af2b35798aa7d72047f4c8ffa
Best regards,
--
Bjorn Andersson <[email protected]>
On 21-07-23, 17:24, Manivannan Sadhasivam wrote:
> On Fri, Jul 21, 2023 at 03:12:06PM +0530, Viresh Kumar wrote:
> > On 20-07-23, 11:10, Manivannan Sadhasivam wrote:
> > > Hi,
> > >
> > > This series adds OPP (Operating Points) support to UFSHCD driver and
> > > interconnect support to Qcom UFS driver.
> > >
> > > Motivation behind adding OPP support is to scale both clocks as well as
> > > regulators/performance state dynamically. Currently, UFSHCD just scales
> > > clock frequency during runtime with the help of "freq-table-hz" property
> > > defined in devicetree. With the addition of OPP tables in devicetree (as
> > > done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> > > both clocks and performance state of power domain which helps in power
> > > saving.
> > >
> > > For the addition of OPP support to UFSHCD, there are changes required to
> > > the OPP framework and devfreq drivers which are also added in this series.
> > >
> > > Finally, interconnect support is added to Qcom UFS driver for scaling the
> > > interconnect path dynamically. This is required to avoid boot crash in
> > > recent SoCs and also to save power during runtime. More information is
> > > available in patch 13/13.
> >
> > Hi Mani,
> >
> > I have picked the OPP related patches from here (apart from DT one)
> > and sent them separately in a series, along with few changes from me.
> > Also pushed them in my linux-next branch.
> >
>
> Thanks Viresh! For patch 8/15, Kbuild bot has identified one potential null ptr
> dereference issue. Could you please fix that in your branch?
>
> You just need to remove the opp dereference in dev_pm_opp_get_freq_indexed()
> before the IS_ERR_OR_NULL() check as below:
>
> ```
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 66dc0d0cfaed..683e6e61f80b 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -208,9 +208,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
> */
> unsigned long dev_pm_opp_get_freq_indexed(struct dev_pm_opp *opp, u32 index)
> {
> - struct opp_table *opp_table = opp->opp_table;
> -
> - if (IS_ERR_OR_NULL(opp) || index >= opp_table->clk_count) {
> + if (IS_ERR_OR_NULL(opp) || index >= opp->opp_table->clk_count) {
> pr_err("%s: Invalid parameters\n", __func__);
> return 0;
> }
Fixed.
--
viresh