| Message ID | 20230720054100.9940-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | UFS: Add OPP and interconnect support | expand |
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.
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,
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.
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/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/ [2] https://lore.kernel.org/all/20221117104957.254648-1-bmasney@redhat.com/ 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(-)