| Message ID | 20230712103213.101770-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | UFS: Add OPP and interconnect support | expand |
On 12-07-23, 19:48, Dmitry Baryshkov wrote: > On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > > > + struct dev_pm_opp *opp, void *data, > > > > + bool scaling_down) > > > > +{ > > > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > > > + struct list_head *head = &hba->clk_list_head; > > > > + struct ufs_clk_info *clki; > > > > + unsigned long freq; > > > > + u8 idx = 0; > > > > + int ret; > > > > + > > > > + list_for_each_entry(clki, head, list) { > > > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > > > + > > > > + /* Do not set rate for clocks having frequency as 0 */ > > > > + if (!freq) > > > > + continue; > > > > > > Can we omit these clocks from the opp table? I don't think they serve any > > > purpose. > > > > > > > No, we cannot. OPP requires the clocks and opp-hz to be of same length. I am okay with having a patch for the OPP core to modify this behavior, as I told privately earlier. > > And we > > cannot omit those clocks as well since linux needs to gate control them. > > Hmm, I thought we push the list of "interesting" clocks through > config->clock_names. Yes, another way to solve this would be keep the interesting clocks in the beginning in "clock-names" field and let the platform pass only those to the OPP core. > > > > > Maybe it would even make sense to move this function to drivers/opp then, as > > > it will be generic enough. > > > > > > > There is already a generic function available in OPP core. But we cannot use it > > as we need to skip setting 0 freq and that's not applicable in OPP core as > > discussed with Viresh offline. > > Ack. I am okay with either of the solutions, it is for you guys to decide what works better for your platform.
On Thu, Jul 13, 2023 at 09:39:18AM +0530, Viresh Kumar wrote: > On 12-07-23, 19:48, Dmitry Baryshkov wrote: > > On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > > > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > > > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > > > > + struct dev_pm_opp *opp, void *data, > > > > > + bool scaling_down) > > > > > +{ > > > > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > > > > + struct list_head *head = &hba->clk_list_head; > > > > > + struct ufs_clk_info *clki; > > > > > + unsigned long freq; > > > > > + u8 idx = 0; > > > > > + int ret; > > > > > + > > > > > + list_for_each_entry(clki, head, list) { > > > > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > > > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > > > > + > > > > > + /* Do not set rate for clocks having frequency as 0 */ > > > > > + if (!freq) > > > > > + continue; > > > > > > > > Can we omit these clocks from the opp table? I don't think they serve any > > > > purpose. > > > > > > > > > > No, we cannot. OPP requires the clocks and opp-hz to be of same length. > > I am okay with having a patch for the OPP core to modify this > behavior, as I told privately earlier. > > > > And we > > > cannot omit those clocks as well since linux needs to gate control them. > > > > Hmm, I thought we push the list of "interesting" clocks through > > config->clock_names. > > Yes, another way to solve this would be keep the interesting clocks in > the beginning in "clock-names" field and let the platform pass only > those to the OPP core. > > > > > > > > Maybe it would even make sense to move this function to drivers/opp then, as > > > > it will be generic enough. > > > > > > > > > > There is already a generic function available in OPP core. But we cannot use it > > > as we need to skip setting 0 freq and that's not applicable in OPP core as > > > discussed with Viresh offline. > > > > Ack. > > I am okay with either of the solutions, it is for you guys to decide > what works better for your platform. > We can settle with this custom callback for now. If there are drivers in the future trying to do the same (skipping 0 freq) then we can generalize. - Mani > -- > viresh
On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > We can settle with this custom callback for now. If there are drivers in the > future trying to do the same (skipping 0 freq) then we can generalize. Just for completeness, there isn't much to generalize here apart from changing the DT order of clocks. Isn't it ? The change require for the OPP core makes sense, I will probably just push it anyway.
On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote: > On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > > We can settle with this custom callback for now. If there are drivers in the > > future trying to do the same (skipping 0 freq) then we can generalize. > > Just for completeness, there isn't much to generalize here apart from > changing the DT order of clocks. Isn't it ? > Even with changing the order, driver has to know the "interesting" clocks beforehand. But that varies between platforms (this is a generic driver for ufshc platforms). And I do not know if clocks have any dependency between them, atleast not in Qcom platforms. But not sure about others. - Mani > The change require for the OPP core makes sense, I will probably just > push it anyway. > > -- > viresh
On Thu, Jul 13, 2023 at 11:13:02AM +0530, Viresh Kumar wrote: > Okay, sorry about missing one point first. I thought we are adding the > clk config callback (which neglects 0 frequencies) to a Qcom only > driver and so was okay-ish with that. But now that I realize that this > is a generic driver instead (my mistake here), I wonder if it is the > right thing to do anymore. > That's the pre-opp behavior as well. Reason is, most of the platforms have only gate clocks supplied to the ufs controller and cannot change the frequency. Only Qcom requires changing the frequency of _some_ clocks, so that's why we have to use this hack of skipping 0 freq clocks. > On 13-07-23, 10:58, Manivannan Sadhasivam wrote: > > On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote: > > > On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > > > > We can settle with this custom callback for now. If there are drivers in the > > > > future trying to do the same (skipping 0 freq) then we can generalize. > > > > > > Just for completeness, there isn't much to generalize here apart from > > > changing the DT order of clocks. Isn't it ? > > > > > > > Even with changing the order, driver has to know the "interesting" clocks > > beforehand. But that varies between platforms (this is a generic driver for > > ufshc platforms). > > > > And I do not know if clocks have any dependency between them, atleast not in > > Qcom platforms. But not sure about others. > > Maybe this requires some sort of callback, per-platform, which gets > you these details or the struct dev_pm_opp_config itself (so platforms > can choose the callback too, in case order is important). > Yeah but that seems overkill since the current config_clks helper satisfies the requirement. - Mani > > > The change require for the OPP core makes sense, I will probably just > > > push it anyway. > > I tried to look at this code and I think it is doing the right thing > currently, i.e. it matches clk-count with the number of frequencies in > opp-hz, which should turn out to be the same in your case. So nothing > to change here I guess. > > -- > viresh
On 12.07.2023 12:32, Manivannan Sadhasivam wrote: > UFS host controller requires interconnect path configuration for proper > working. So let's specify them for SM8250 SoC. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 72fd66db9c51..7a495ff7512f 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -2197,6 +2197,10 @@ ufs_mem_hc: ufshc@1d84000 { > > operating-points-v2 = <&ufs_opp_table>; > > + interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>, > + <&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>; > + interconnect-names = "ufs-ddr", "cpu-ufs"; > + > status = "disabled"; > > ufs_opp_table: opp-table {
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/ Krzysztof Kozlowski (2): dt-bindings: ufs: common: add OPP table arm64: dts: qcom: sdm845: Add OPP table support to UFSHC Manivannan Sadhasivam (12): 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: 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 | 142 ++++++++++++++---- drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++- drivers/ufs/host/ufs-qcom.h | 3 + drivers/ufs/host/ufshcd-pltfrm.c | 116 ++++++++++++++ include/linux/pm_opp.h | 26 ++++ include/ufs/ufshcd.h | 4 + 12 files changed, 574 insertions(+), 64 deletions(-)