| Message ID | 1586353607-32222-2-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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
On 4/9/2020 1:27 PM, Viresh Kumar wrote: > 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 ? yes, freq and perf state. > >> while just needing to scale the clock on some others. > > And only freq here ? yes. > >> 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 > ? These are existing devices already upstream. > >> 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 ? Well, it is to drop votes. But in cases where we don't have perf votes being put (and only clock is scaled), the driver would still call this with freq = 0, i am just making sure that in such cases its treated as a nop. > >> } 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. 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) 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/ > >> 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 >
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 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 > while just needing to scale the clock on some others. > > 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. > > 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(+) Applied. Thanks.
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; } 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; + } + temp_freq = old_freq; old_opp = _find_freq_ceil(opp_table, &temp_freq); if (IS_ERR(old_opp)) {
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 while just needing to scale the clock on some others. 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. 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(+)