| Message ID | 20230731163357.49045-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | UFS: Add OPP support | expand |
On 31/07/2023 19:33, Manivannan Sadhasivam wrote: > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > UFS host controller, when scaling gears, should choose appropriate > performance state of RPMh power domain controller along with clock > frequency. So let's add the OPP table support to specify both clock > frequency and RPMh performance states replacing the old "freq-table-hz" > property. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > [mani: Splitted pd change and used rpmhpd_opp_low_svs] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index 055ca80c0075..2ea6eb44953e 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -2605,22 +2605,44 @@ ufs_mem_hc: ufshc@1d84000 { > <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, > <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, > <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; > - freq-table-hz = > - <50000000 200000000>, > - <0 0>, > - <0 0>, > - <37500000 150000000>, > - <0 0>, > - <0 0>, > - <0 0>, > - <0 0>, > - <75000000 300000000>; > + > + operating-points-v2 = <&ufs_opp_table>; > > interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mem_noc SLAVE_EBI1 0>, > <&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>; > interconnect-names = "ufs-ddr", "cpu-ufs"; > > status = "disabled"; > + > + ufs_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-50000000 { > + opp-hz = /bits/ 64 <50000000>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <37500000>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <75000000>; > + required-opps = <&rpmhpd_opp_low_svs>; > + }; I'd say, I'm still slightly unhappy about the 0 clock rates here. We need only three clocks here: core, core_clk_unipro and optional ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only these two or three clock names to devm_pm_opp_set_config() ? The OPP core doesn't need to know about all the rest of the clocks. > + > + opp-200000000 { > + opp-hz = /bits/ 64 <200000000>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <150000000>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <0>, > + /bits/ 64 <300000000>; > + required-opps = <&rpmhpd_opp_nom>; > + }; > + }; > }; > > ufs_mem_phy: phy@1d87000 {
On Mon, Sep 11, 2023 at 04:15:10PM +0300, Dmitry Baryshkov wrote: > On 31/07/2023 19:33, Manivannan Sadhasivam wrote: > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > UFS host controller, when scaling gears, should choose appropriate > > performance state of RPMh power domain controller along with clock > > frequency. So let's add the OPP table support to specify both clock > > frequency and RPMh performance states replacing the old "freq-table-hz" > > property. > > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > [mani: Splitted pd change and used rpmhpd_opp_low_svs] > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++++++++++++++++++++------- > > 1 file changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > index 055ca80c0075..2ea6eb44953e 100644 > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > @@ -2605,22 +2605,44 @@ ufs_mem_hc: ufshc@1d84000 { > > <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, > > <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, > > <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; > > - freq-table-hz = > > - <50000000 200000000>, > > - <0 0>, > > - <0 0>, > > - <37500000 150000000>, > > - <0 0>, > > - <0 0>, > > - <0 0>, > > - <0 0>, > > - <75000000 300000000>; > > + > > + operating-points-v2 = <&ufs_opp_table>; > > interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mem_noc SLAVE_EBI1 0>, > > <&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>; > > interconnect-names = "ufs-ddr", "cpu-ufs"; > > status = "disabled"; > > + > > + ufs_opp_table: opp-table { > > + compatible = "operating-points-v2"; > > + > > + opp-50000000 { > > + opp-hz = /bits/ 64 <50000000>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <37500000>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <75000000>; > > + required-opps = <&rpmhpd_opp_low_svs>; > > + }; > > I'd say, I'm still slightly unhappy about the 0 clock rates here. Neither do I. But it is the only viable option I could found. > We need only three clocks here: core, core_clk_unipro and optional > ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only > these two or three clock names to devm_pm_opp_set_config() ? The OPP core > doesn't need to know about all the rest of the clocks. > We need to enable/disable all of the clocks, but only need to control the rate for these 3 clocks. So we cannot just use 3 clocks. If the OPP table has only 3 entries (omitting the gate-only clocks), then we need some hack in the driver to match the rates against the clock entries. Doing so will result in hardcoding the clock info in the driver which I do not want to do. If we have something like "opp-hz-names" to relate the rates to clock-names, it might do the job. But it needs some input from Viresh. - Mani > > + > > + opp-200000000 { > > + opp-hz = /bits/ 64 <200000000>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <150000000>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <0>, > > + /bits/ 64 <300000000>; > > + required-opps = <&rpmhpd_opp_nom>; > > + }; > > + }; > > }; > > ufs_mem_phy: phy@1d87000 { > > -- > With best wishes > Dmitry >
Hi, This series adds OPP (Operating Points) support to UFSHCD 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. The OPP framework changes are already merged and available in linux-next, the devfreq change is added in this series. Credits ======= This series is a continuation of previous work by Krzysztof Kozlowski [1]. 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. Dependency ========== This series depends on the OPP framework changes that got merged into PM tree and available in linux-next. Thanks, Mani [1] https://lore.kernel.org/all/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/ Changes in v3: * Rebased on top of linux-next/master tag: next-20230731 * Dropped the already applied patches (dts, opp binding and framework) * Moved the interconnect patches to a separate series: https://lore.kernel.org/linux-scsi/20230731145020.41262-1-manivannan.sadhasivam@linaro.org/ * Moved ufshcd_opp_config_clks() API to ufshcd.c to fix the build failure reported by Kbuild bot: https://lore.kernel.org/all/202307210542.KoLHRbU6-lkp@intel.com/ * Collected Acks * v2: https://lore.kernel.org/all/20230720054100.9940-1-manivannan.sadhasivam@linaro.org/ 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 (4): 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: sm8250: Add OPP table support to UFSHC .../devicetree/bindings/ufs/ufs-common.yaml | 34 +++- arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++- arch/arm64/boot/dts/qcom/sm8250.dtsi | 39 +++- drivers/devfreq/devfreq.c | 14 +- drivers/ufs/core/ufshcd.c | 179 ++++++++++++++---- drivers/ufs/host/ufshcd-pltfrm.c | 78 ++++++++ include/ufs/ufshcd.h | 7 + 7 files changed, 331 insertions(+), 62 deletions(-) base-commit: ec89391563792edd11d138a853901bce76d11f44 prerequisite-patch-id: 9e7233b8c34b4eeef63a90e851bc9429619627bc prerequisite-patch-id: a18af123ce0e9537a96676ae0b2d8c1f0fd19c4b