| Message ID | 20230720054100.9940-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | UFS: Add OPP and interconnect support | expand |
Hi, On 23. 7. 20. 14:40, Manivannan Sadhasivam wrote: > Some devfreq consumers like UFS driver need to work with multiple clocks > through the OPP framework. For this reason, OPP framework exposes the > _indexed() APIs for finding the floor/ceil of the supplied frequency of > the indexed clock. So let's use them in the devfreq driver. > > Currently, the clock index of 0 is used which works fine for multiple as > well as single clock. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/devfreq/devfreq.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index e36cbb920ec8..7686993d639f 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -88,7 +88,7 @@ static unsigned long find_available_min_freq(struct devfreq *devfreq) > struct dev_pm_opp *opp; > unsigned long min_freq = 0; > > - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq); > + opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &min_freq, 0); This patch changed the used function from dev_pm_opp_find_freq_ceil to dev_pm_opp_find_freq_ceil_indexed even if there are no supporting of the multiple clocks and then dev_pm_opp_find_freq_ceil is not removed from OPP. I think that it is better to use dev_pm_opp_find_freq_ceil_indexed when need to support multiple clocks with real case. > if (IS_ERR(opp)) > min_freq = 0; > else > @@ -102,7 +102,7 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > struct dev_pm_opp *opp; > unsigned long max_freq = ULONG_MAX; > > - opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq); > + opp = dev_pm_opp_find_freq_floor_indexed(devfreq->dev.parent, &max_freq, 0); > if (IS_ERR(opp)) > max_freq = 0; > else > @@ -196,7 +196,7 @@ static int set_freq_table(struct devfreq *devfreq) > return -ENOMEM; > > for (i = 0, freq = 0; i < devfreq->max_state; i++, freq++) { > - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq); > + opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &freq, 0); > if (IS_ERR(opp)) { > devm_kfree(devfreq->dev.parent, devfreq->freq_table); > return PTR_ERR(opp); > @@ -2034,18 +2034,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, > > if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) { > /* The freq is an upper bound. opp should be lower */ > - opp = dev_pm_opp_find_freq_floor(dev, freq); > + opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0); > > /* If not available, use the closest opp */ > if (opp == ERR_PTR(-ERANGE)) > - opp = dev_pm_opp_find_freq_ceil(dev, freq); > + opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0); > } else { > /* The freq is an lower bound. opp should be higher */ > - opp = dev_pm_opp_find_freq_ceil(dev, freq); > + opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0); > > /* If not available, use the closest opp */ > if (opp == ERR_PTR(-ERANGE)) > - opp = dev_pm_opp_find_freq_floor(dev, freq); > + opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0); > } > > return opp;
On Mon, Jul 24, 2023 at 05:06:04AM +0900, Chanwoo Choi wrote: > Hi, > > On 23. 7. 20. 14:40, Manivannan Sadhasivam wrote: > > Some devfreq consumers like UFS driver need to work with multiple clocks > > through the OPP framework. For this reason, OPP framework exposes the > > _indexed() APIs for finding the floor/ceil of the supplied frequency of > > the indexed clock. So let's use them in the devfreq driver. > > > > Currently, the clock index of 0 is used which works fine for multiple as > > well as single clock. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/devfreq/devfreq.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index e36cbb920ec8..7686993d639f 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -88,7 +88,7 @@ static unsigned long find_available_min_freq(struct devfreq *devfreq) > > struct dev_pm_opp *opp; > > unsigned long min_freq = 0; > > > > - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq); > > + opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &min_freq, 0); > > This patch changed the used function from dev_pm_opp_find_freq_ceil > to dev_pm_opp_find_freq_ceil_indexed even if there are no supporting of the multiple clocks > and then dev_pm_opp_find_freq_ceil is not removed from OPP. > > I think that it is better to use dev_pm_opp_find_freq_ceil_indexed > when need to support multiple clocks with real case. > There is the user for dev_pm_opp_find_freq_ceil_indexed() which is the UFS driver and since UFS is using devfreq, we need this change. I've added this info in the commit message as well. What am I missing? - Mani > > if (IS_ERR(opp)) > > min_freq = 0; > > else > > @@ -102,7 +102,7 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > > struct dev_pm_opp *opp; > > unsigned long max_freq = ULONG_MAX; > > > > - opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq); > > + opp = dev_pm_opp_find_freq_floor_indexed(devfreq->dev.parent, &max_freq, 0); > > if (IS_ERR(opp)) > > max_freq = 0; > > else > > @@ -196,7 +196,7 @@ static int set_freq_table(struct devfreq *devfreq) > > return -ENOMEM; > > > > for (i = 0, freq = 0; i < devfreq->max_state; i++, freq++) { > > - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq); > > + opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &freq, 0); > > if (IS_ERR(opp)) { > > devm_kfree(devfreq->dev.parent, devfreq->freq_table); > > return PTR_ERR(opp); > > @@ -2034,18 +2034,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, > > > > if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) { > > /* The freq is an upper bound. opp should be lower */ > > - opp = dev_pm_opp_find_freq_floor(dev, freq); > > + opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0); > > > > /* If not available, use the closest opp */ > > if (opp == ERR_PTR(-ERANGE)) > > - opp = dev_pm_opp_find_freq_ceil(dev, freq); > > + opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0); > > } else { > > /* The freq is an lower bound. opp should be higher */ > > - opp = dev_pm_opp_find_freq_ceil(dev, freq); > > + opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0); > > > > /* If not available, use the closest opp */ > > if (opp == ERR_PTR(-ERANGE)) > > - opp = dev_pm_opp_find_freq_floor(dev, freq); > > + opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0); > > } > > > > return opp; > > -- > Best Regards, > Samsung Electronics > Chanwoo Choi >
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.
On 23. 7. 24. 14:46, Manivannan Sadhasivam wrote: > On Mon, Jul 24, 2023 at 05:06:04AM +0900, Chanwoo Choi wrote: >> Hi, >> >> On 23. 7. 20. 14:40, Manivannan Sadhasivam wrote: >>> Some devfreq consumers like UFS driver need to work with multiple clocks >>> through the OPP framework. For this reason, OPP framework exposes the >>> _indexed() APIs for finding the floor/ceil of the supplied frequency of >>> the indexed clock. So let's use them in the devfreq driver. >>> >>> Currently, the clock index of 0 is used which works fine for multiple as >>> well as single clock. >>> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >>> --- >>> drivers/devfreq/devfreq.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index e36cbb920ec8..7686993d639f 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -88,7 +88,7 @@ static unsigned long find_available_min_freq(struct devfreq *devfreq) >>> struct dev_pm_opp *opp; >>> unsigned long min_freq = 0; >>> >>> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq); >>> + opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &min_freq, 0); >> >> This patch changed the used function from dev_pm_opp_find_freq_ceil >> to dev_pm_opp_find_freq_ceil_indexed even if there are no supporting of the multiple clocks >> and then dev_pm_opp_find_freq_ceil is not removed from OPP. >> >> I think that it is better to use dev_pm_opp_find_freq_ceil_indexed >> when need to support multiple clocks with real case. >> > > There is the user for dev_pm_opp_find_freq_ceil_indexed() which is the UFS > driver and since UFS is using devfreq, we need this change. I've added this info > in the commit message as well. What am I missing? I found out the difference of them. - dev_pm_opp_find_freq_ceil() used the 'assert_single_clk' which check the count of clock. (snip) Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
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(-)