| Message ID | 1586353607-32222-1-git-send-email-rnayak@codeaurora.org |
|---|---|
| Headers | show |
| Series | DVFS for IO devices on sdm845 and sc7180 | expand |
On 08-04-20, 19:16, Rajendra Nayak wrote: > With OPP core now supporting DVFS for IO devices, we have instances of > IO devices (same IP block) which require an OPP on some platforms/SoCs By OPP you mean both freq and voltage here ? > while just needing to scale the clock on some others. And only freq here ? > In order to avoid conditional code in every driver which supports such > devices (to check for availability of OPPs and then deciding to do > either dev_pm_opp_set_rate() or clk_set_rate()) add support to manage > empty OPP tables with a clk handle. Why can't these devices have an opp table with just rate mentioned in each node ? > This makes dev_pm_opp_set_rate() equivalent of a clk_set_rate() for > devices with just a clk and no OPPs specified, and makes > dev_pm_opp_set_rate(0) bail out without throwing an error. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/opp/core.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index ba43e6a..e4f01e7 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -819,6 +819,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > if (unlikely(!target_freq)) { > if (opp_table->required_opp_tables) { > ret = _set_required_opps(dev, opp_table, NULL); > + } else if (!_get_opp_count(opp_table)) { > + return 0; Why should anyone call this with target_freq = 0 ? I know it was required to drop votes in the above case, but why here ? > } else { > dev_err(dev, "target frequency can't be 0\n"); > ret = -EINVAL; > @@ -849,6 +851,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > goto put_opp_table; > } > > + /* > + * For IO devices which require an OPP on some platforms/SoCs > + * while just needing to scale the clock on some others > + * we look for empty OPP tables with just a clock handle and > + * scale only the clk. This makes dev_pm_opp_set_rate() > + * equivalent to a clk_set_rate() > + */ > + if (!_get_opp_count(opp_table)) { > + ret = _generic_set_opp_clk_only(dev, clk, freq); > + goto put_opp_table; > + } > + Is this enough? _of_add_opp_table_v2() returns with error if there is no OPP node within the table. Please give an example of how DT looks for the case you want to support. > temp_freq = old_freq; > old_opp = _find_freq_ceil(opp_table, &temp_freq); > if (IS_ERR(old_opp)) { > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
Hi Rajendra, On Wed, Apr 08, 2020 at 07:16:29PM +0530, Rajendra Nayak wrote: > geni spi needs to express a perforamnce state requirement on CX > depending on the frequency of the clock rates. Use OPP table from > DT to register with OPP framework and use dev_pm_opp_set_rate() to > set the clk/perf state. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Cc: Alok Chauhan <alokc@codeaurora.org> > Cc: Akash Asthana <akashast@codeaurora.org> > Cc: linux-spi@vger.kernel.org > --- > drivers/spi/spi-geni-qcom.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index c397242..ce387dc 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -7,6 +7,7 @@ > #include <linux/log2.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/pm_opp.h> > #include <linux/pm_runtime.h> > #include <linux/qcom-geni-se.h> > #include <linux/spi/spi.h> > @@ -95,7 +96,6 @@ static int get_spi_clk_cfg(unsigned int speed_hz, > { > unsigned long sclk_freq; > unsigned int actual_hz; > - struct geni_se *se = &mas->se; > int ret; > > ret = geni_se_clk_freq_match(&mas->se, > @@ -112,9 +112,9 @@ static int get_spi_clk_cfg(unsigned int speed_hz, > > dev_dbg(mas->dev, "req %u=>%u sclk %lu, idx %d, div %d\n", speed_hz, > actual_hz, sclk_freq, *clk_idx, *clk_div); > - ret = clk_set_rate(se->clk, sclk_freq); > + ret = dev_pm_opp_set_rate(mas->dev, sclk_freq); > if (ret) > - dev_err(mas->dev, "clk_set_rate failed %d\n", ret); > + dev_err(mas->dev, "dev_pm_opp_set_rate failed %d\n", ret); > return ret; > } > > @@ -553,6 +553,7 @@ static int spi_geni_probe(struct platform_device *pdev) > if (!spi) > return -ENOMEM; > > + > platform_set_drvdata(pdev, spi); > mas = spi_master_get_devdata(spi); > mas->irq = irq; > @@ -561,6 +562,8 @@ static int spi_geni_probe(struct platform_device *pdev) > mas->se.wrapper = dev_get_drvdata(dev->parent); > mas->se.base = base; > mas->se.clk = clk; > + mas->se.opp = dev_pm_opp_set_clkname(&pdev->dev, "se"); As commented on the serial patch, it seems an error check is needed and the OPP table saved in 'struct geni_se' is never used. > + dev_pm_opp_of_add_table(&pdev->dev); This function could also fail for multiple reasons, so the return value should be checked.
On 13-04-20, 16:04, Rajendra Nayak wrote: > FWIK, no one should call a _of_add_opp_table_v2 in cases where there is no OPP in DT? > The 'empty' OPP table from what I understand will be created by dev_pm_opp_set_clkname. > A good case to look at is the PATCH 13/21 in this series. The driver I am modifying > is used on sdm845/sc7180 and a host of other older qualcomm SoCs. Since i am adding > support for perf state voting using OPP only on sdm845/sc7180 I want the existing > platforms to just do what they were doing. Now thats not possible unless I start > adding a bunch of if/else around every opp call in the driver to distinguish between > the two. > > I am a little surprised since I though the idea of doing something like this came from > you :) (or perhaps Stephen, I somehow can't recollect) Me only as I start remembering it now :) > to avoid all the if/else conditions > I had when I initially posted some of these changes. > Btw, you had this patch reviewed when this was posted a long while back too [1] > > [1] https://patchwork.kernel.org/patch/11027217/ That's an year back, in my defence :) But anyway, I wasn't opposed to the idea now as well. I was just making sure all things are handled well :)
On Wed, 15 Apr 2020 at 18:43, Rajendra Nayak <rnayak@codeaurora.org> wrote: > > > > On 4/15/2020 7:22 PM, Ulf Hansson wrote: > > On Wed, 8 Apr 2020 at 15:48, Rajendra Nayak <rnayak@codeaurora.org> wrote: > >> > >> On some qualcomm SoCs we need to vote on a performance state of a power > >> domain depending on the clock rates. Hence move to using OPP api to set > >> the clock rate and performance state specified in the OPP table. > >> On platforms without an OPP table, dev_pm_opp_set_rate() is eqvivalent to > >> clk_set_rate() > >> > >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > >> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >> Cc: Pradeep P V K <ppvk@codeaurora.org> > >> Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > >> Cc: Subhash Jadavani <subhashj@codeaurora.org> > >> Cc: linux-mmc@vger.kernel.org > > > > This looks good to me! > > > > However, are there any of the other patches in the series that > > $subject patch depends on - or can I apply this as a standalone mmc > > patch? > > Hey Ulf, thanks for the review. I'll just need to respin these to make > sure I do not do a dev_pm_opp_of_remove_table() if dev_pm_opp_of_add_table() > isn;t successful as discussed with Viresh on another thread [1] > > As for the dependencies, its only PATCH 01/21 in this series and that's > already been queued by Viresh [2] I see, thanks! Looks like Viresh is sending it to be included for v5.7-rc2, so I can pick your new version of $subject patch next week. [...] Kind regards Uffe
We have had support added in the OPP core for a while now to support DVFS for IO devices, and this series uses that infrastructure to add DVFS support for various IO devices in sdm845 and sc7180 SoCs. Rajendra Nayak (21): opp: Manage empty OPP tables with clk handle tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state spi: spi-geni-qcom: Use OPP API to set clk/perf state arm64: dts: sdm845: Add OPP table for all qup devices arm64: dts: sc7180: Add OPP table for all qup devices scsi: ufs: Add support to manage multiple power domains in ufshcd-pltfrm scsi: ufs: Add support for specifying OPP tables in DT arm64: dts: sdm845: Add ufs opps and power-domains drm/msm/dpu: Use OPP API to set clk/perf state drm/msm: dsi: Use OPP API to set clk/perf state arm64: dts: sdm845: Add DSI and MDP OPP tables and power-domains arm64: dts: sc7180: Add DSI and MDP OPP tables and power-domains mmc: sdhci-msm: Use OPP API to set clk/perf state arm64: dts: sdm845: Add sdhc opps and power-domains arm64: dts: sc7180: Add sdhc opps and power-domains media: venus: core: Add support for opp tables/perf voting arm64: dts: sdm845: Add OPP tables and power-domains for venus arm64: dts: sc7180: Add OPP tables and power-domains for venus spi: spi-qcom-qspi: Use OPP API to set clk/perf state arm64: dts: sdm845: Add qspi opps and power-domains arm64: dts: sc7180: Add qspi opps and power-domains arch/arm64/boot/dts/qcom/sc7180.dtsi | 199 ++++++++++++++++- arch/arm64/boot/dts/qcom/sdm845.dtsi | 287 ++++++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 + drivers/gpu/drm/msm/dsi/dsi.h | 2 + drivers/gpu/drm/msm/dsi/dsi_cfg.c | 4 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 48 +++++ drivers/media/platform/qcom/venus/core.c | 16 ++ drivers/media/platform/qcom/venus/core.h | 4 + drivers/media/platform/qcom/venus/pm_helpers.c | 37 +++- drivers/mmc/host/sdhci-msm.c | 20 +- drivers/opp/core.c | 14 ++ drivers/scsi/ufs/ufshcd-pltfrm.c | 58 ++++- drivers/scsi/ufs/ufshcd.c | 17 +- drivers/scsi/ufs/ufshcd.h | 3 + drivers/spi/spi-geni-qcom.c | 14 +- drivers/spi/spi-qcom-qspi.c | 10 +- drivers/tty/serial/qcom_geni_serial.c | 20 +- include/linux/qcom-geni-se.h | 2 + 19 files changed, 735 insertions(+), 29 deletions(-)