| Message ID | 20230607124628.157465-14-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_scmi/opp/dvfs: Add generic performance scaling support | expand |
On 07-06-23, 14:46, Ulf Hansson wrote: > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 79b4b44ced3e..81a3418e2eaf 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, > return ret; > } > > + if (opp->provider == DEV_PM_OPP_TYPE_GENPD) { > + ret = dev_pm_genpd_set_performance_state(dev, opp->level); > + if (ret) { > + dev_err(dev, "Failed to set performance level: %d\n", > + ret); > + return ret; > + } > + } > + I don't like this :) We already have these calls in place from within _set_required_opps(), and we should try to get this done in a way that those calls themselves get the performance state configured.
On Thu, 8 Jun 2023 at 07:34, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 07-06-23, 14:46, Ulf Hansson wrote: > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index 79b4b44ced3e..81a3418e2eaf 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, > > return ret; > > } > > > > + if (opp->provider == DEV_PM_OPP_TYPE_GENPD) { > > + ret = dev_pm_genpd_set_performance_state(dev, opp->level); > > + if (ret) { > > + dev_err(dev, "Failed to set performance level: %d\n", > > + ret); > > + return ret; > > + } > > + } > > + > > I don't like this :) > > We already have these calls in place from within _set_required_opps(), and we > should try to get this done in a way that those calls themselves get the > performance state configured. I was looking at that, but wanted to keep things as simple as possible in the $subject series. The required opps are also different, as it's getting parsed from DT both for the genpd provider and the consumer. The point is, there are more code involved but just _set_required_opps(). For example, _set_performance_state() (which is the one that calls dev_pm_genpd_set_performance_state()) is designed to be used for required opps. Does it really make sense to rework _set_performance_state() so it can be used for this case too, just to avoid another call to dev_pm_genpd_set_performance_state() somewhere in the code? One improvement we can make though, is to add a helper function, "_set_opp_level()", which we call from _set_opp(). This can then replace the call to _set_required_opps() and the code above that I am adding for DEV_PM_OPP_TYPE_GENPD. At least that should keep the code _set_opp() a bit more readable. What do you think? Kind regards Uffe
On 08-06-23, 11:37, Ulf Hansson wrote: > The required opps are also different, as it's getting parsed from DT > both for the genpd provider and the consumer. The point is, there are > more code involved but just _set_required_opps(). > > For example, _set_performance_state() (which is the one that calls > dev_pm_genpd_set_performance_state()) is designed to be used for > required opps. Does it really make sense to rework > _set_performance_state() so it can be used for this case too, just to > avoid another call to dev_pm_genpd_set_performance_state() somewhere > in the code? What we need here, in you case, is really the required-opp thing, without the DT parsing. The genpd will have an OPP table here, and devices (you are adding OPP table dynamically for) shall have the genpd's OPPs as their required OPPs, since for setting OPPs of the device, it is *required* to have OPP of the genpd set too. Just like how it happens with DT. No special handling will be required in dev_pm_opp_set_opp() path in this case and existing code will just work. You just need to set the required-opp tables properly.
On Thu, 8 Jun 2023 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 08-06-23, 11:37, Ulf Hansson wrote: > > The required opps are also different, as it's getting parsed from DT > > both for the genpd provider and the consumer. The point is, there are > > more code involved but just _set_required_opps(). > > > > For example, _set_performance_state() (which is the one that calls > > dev_pm_genpd_set_performance_state()) is designed to be used for > > required opps. Does it really make sense to rework > > _set_performance_state() so it can be used for this case too, just to > > avoid another call to dev_pm_genpd_set_performance_state() somewhere > > in the code? > > What we need here, in you case, is really the required-opp thing, without the > DT parsing. The genpd will have an OPP table here, and devices (you are adding > OPP table dynamically for) shall have the genpd's OPPs as their required OPPs, > since for setting OPPs of the device, it is *required* to have OPP of the genpd > set too. Just like how it happens with DT. No special handling will be required > in dev_pm_opp_set_opp() path in this case and existing code will just work. You > just need to set the required-opp tables properly. Okay, if I understand your point you want to avoid creating OPPs for each device, but rather coupling them with the genpd provider's OPP table. Right? Note that, there is no such thing as a "required opp" in the SCMI performance protocol case. A device is compatible to use all of the OPPs that its corresponding SCMI performance domain provides. Should we rename the required opp things in the OPP core to better reflect this too? That said, we still need to be able to add OPPs dynamically when not based on DT. The difference would be that we add the OPPs when initializing the genpd provider instead of when attaching the devices. In other words, we still need something along the lines of the new dev_pm_opp_add_dynamic() API that $subject series is introducing, I think. Moreover, to tie the consumer device's OPP table to their genpd provider's OPP table (call it required-opp or whatever), we need another OPP helper function that we can call from the genpd provider's ->attach_dev() callback. Similarly, we need to be able to remove this connection when genpd's ->detach_dev() callback is invoked. I will think of something here. Finally, I want to point out that there is work going on in parallel with this, that is adding performance state support for the ACPI PM domain. The ACPI PM domain, isn't a genpd provider but implements it's own PM domain. The important point is, that it will have its own variant of the dev_pm_genpd_set_performance_state() that we may need to call from the OPP library. Kind regards Uffe
On 08-06-23, 13:45, Ulf Hansson wrote: > Okay, if I understand your point you want to avoid creating OPPs for > each device, but rather coupling them with the genpd provider's OPP > table. Right? Not exactly :) > Note that, there is no such thing as a "required opp" in the SCMI > performance protocol case. A device is compatible to use all of the > OPPs that its corresponding SCMI performance domain provides. Should > we rename the required opp things in the OPP core to better reflect > this too? Not really :) > That said, we still need to be able to add OPPs dynamically when not > based on DT. The difference would be that we add the OPPs when > initializing the genpd provider instead of when attaching the devices. > In other words, we still need something along the lines of the new > dev_pm_opp_add_dynamic() API that $subject series is introducing, I > think. That's fine. > Moreover, to tie the consumer device's OPP table to their genpd > provider's OPP table (call it required-opp or whatever), we need > another OPP helper function that we can call from the genpd provider's > ->attach_dev() callback. Similarly, we need to be able to remove this > connection when genpd's ->detach_dev() callback is invoked. I will > think of something here. Something like set/link/config_required_opp().. > Finally, I want to point out that there is work going on in parallel > with this, that is adding performance state support for the ACPI PM > domain. The ACPI PM domain, isn't a genpd provider but implements it's > own PM domain. The important point is, that it will have its own > variant of the dev_pm_genpd_set_performance_state() that we may need > to call from the OPP library. Okay. Let me explain how structures are linked currently with help of below (badly made) drawing. The 'level' fields are only set for Genpd's OPP entries and not devices. The genpd OPP table normally has only this information, where it presents all the possible level values, separate OPP for each of them. Now the devices don't have `level` set in their OPP table, DT or in C structures. All they have are links for required OPPs, which help us find the required level or performance state for a particular genpd. +-----------------+ +------------------+ | Device A | | GENPD OPP Table | +-----------------+ required-opps +------------------+ required-opps |OPP1: freq: x1 +--------------------+ OPP1: +--------------+ +-----------------+ | level: 1 | | |OPP2: freq: y1 +-----------+ +------------------+ | +-----------------+ | | OPP2: +---------+ | |OPP3: freq: z1 +--------+ +--------+ level: 2 | | | +-----------------+ | +------------------+ | | | | OPP3: +--+ | | | | level: 3 | | | | | +------------------+ | | | +-----------------+ | | OPP4: | | | | | Device B | +-----------+ level: 4 | | | | +-----------------+ +------------------+ | | | |OPP1: freq: x2 +------------------------------------------+ | | +-----------------+ | | |OPP2: freq: y2 +-------------------------------------------------+ | +-----------------+ | |OPP3: freq: z2 +------------------------------------------------------+ +-----------------+ What I am asking you to do now is, create an OPP table for the Genpd first, with OPPs for each possible level. Now the Genpd layer creates the OPP table for Device A, where it won't fill the levels, but all other fields and then use a new API to add required OPPs for the device's OPP, which will point into Genpd's OPP entries. And with that the existing code will work fine without any other modifications. Does this help ? -- viresh
On Fri, 9 Jun 2023 at 07:10, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 08-06-23, 13:45, Ulf Hansson wrote: > > Okay, if I understand your point you want to avoid creating OPPs for > > each device, but rather coupling them with the genpd provider's OPP > > table. Right? > > Not exactly :) > > > Note that, there is no such thing as a "required opp" in the SCMI > > performance protocol case. A device is compatible to use all of the > > OPPs that its corresponding SCMI performance domain provides. Should > > we rename the required opp things in the OPP core to better reflect > > this too? > > Not really :) I think it may be confusing, but okay, let's leave it as is. > > > That said, we still need to be able to add OPPs dynamically when not > > based on DT. The difference would be that we add the OPPs when > > initializing the genpd provider instead of when attaching the devices. > > In other words, we still need something along the lines of the new > > dev_pm_opp_add_dynamic() API that $subject series is introducing, I > > think. > > That's fine. > > > Moreover, to tie the consumer device's OPP table to their genpd > > provider's OPP table (call it required-opp or whatever), we need > > another OPP helper function that we can call from the genpd provider's > > ->attach_dev() callback. Similarly, we need to be able to remove this > > connection when genpd's ->detach_dev() callback is invoked. I will > > think of something here. > > Something like set/link/config_required_opp().. > > > Finally, I want to point out that there is work going on in parallel > > with this, that is adding performance state support for the ACPI PM > > domain. The ACPI PM domain, isn't a genpd provider but implements it's > > own PM domain. The important point is, that it will have its own > > variant of the dev_pm_genpd_set_performance_state() that we may need > > to call from the OPP library. > > Okay. > > Let me explain how structures are linked currently with help of below (badly > made) drawing. The 'level' fields are only set for Genpd's OPP entries and not > devices. The genpd OPP table normally has only this information, where it > presents all the possible level values, separate OPP for each of them. > > Now the devices don't have `level` set in their OPP table, DT or in C > structures. All they have are links for required OPPs, which help us find the > required level or performance state for a particular genpd. > > +-----------------+ +------------------+ > | Device A | | GENPD OPP Table | > +-----------------+ required-opps +------------------+ required-opps > |OPP1: freq: x1 +--------------------+ OPP1: +--------------+ > +-----------------+ | level: 1 | | > |OPP2: freq: y1 +-----------+ +------------------+ | > +-----------------+ | | OPP2: +---------+ | > |OPP3: freq: z1 +--------+ +--------+ level: 2 | | | > +-----------------+ | +------------------+ | | > | | OPP3: +--+ | | > | | level: 3 | | | | > | +------------------+ | | | > +-----------------+ | | OPP4: | | | | > | Device B | +-----------+ level: 4 | | | | > +-----------------+ +------------------+ | | | > |OPP1: freq: x2 +------------------------------------------+ | | > +-----------------+ | | > |OPP2: freq: y2 +-------------------------------------------------+ | > +-----------------+ | > |OPP3: freq: z2 +------------------------------------------------------+ > +-----------------+ > Thanks for taking the time to explain things further! It's not entirely easy to follow all the things in the OPP library. However, I think you are mixing up the "level" field with the "pstate" field in the struct dev_pm_opp. The pstate field is solely for genpd and the required opps, while level is *generic* for all types of devices. Right? More precisely, _read_opp_key() parses for the "opp-level" property from DT to set the ->level field. Additionally, the generic OPP helpers functions, like dev_pm_opp_get_level(), dev_pm_opp_find_level_exact(), dev_pm_opp_find_level_ceil() allows any type of consumer driver to operate on the level for an OPP for its device. Please have a look at the apple-soc-cpufreq, for example. > > What I am asking you to do now is, create an OPP table for the Genpd first, with > OPPs for each possible level. Now the Genpd layer creates the OPP table for > Device A, where it won't fill the levels, but all other fields and then use a > new API to add required OPPs for the device's OPP, which will point into Genpd's > OPP entries. And with that the existing code will work fine without any other > modifications. > > Does this help ? Well, I think what you propose may be doable, but I fail to understand the benefit of implementing it like that. As I said earlier, there is no such thing as a required OPP for the SCMI performance domain and neither are the OPP tables described in DT. Moreover, as I understand it, we already have the generic "level" to use, why not use it here? Kind regards Uffe
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 79b4b44ced3e..81a3418e2eaf 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, return ret; } + if (opp->provider == DEV_PM_OPP_TYPE_GENPD) { + ret = dev_pm_genpd_set_performance_state(dev, opp->level); + if (ret) { + dev_err(dev, "Failed to set performance level: %d\n", + ret); + return ret; + } + } + ret = _set_opp_bw(opp_table, opp, dev); if (ret) { dev_err(dev, "Failed to set bw: %d\n", ret); @@ -1155,6 +1164,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, return ret; } + if (opp->provider == DEV_PM_OPP_TYPE_GENPD) { + ret = dev_pm_genpd_set_performance_state(dev, opp->level); + if (ret) { + dev_err(dev, "Failed to set performance level: %d\n", + ret); + return ret; + } + } + ret = _set_required_opps(dev, opp_table, opp, false); if (ret) { dev_err(dev, "Failed to set required opps: %d\n", ret); @@ -1955,6 +1973,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev, /* populate the opp table */ new_opp->rates[0] = opp->freq; new_opp->level = opp->level; + new_opp->provider = opp->provider; tol = u_volt * opp_table->voltage_tolerance_v1 / 100; new_opp->supplies[0].u_volt = u_volt; new_opp->supplies[0].u_volt_min = u_volt - tol; diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index b15770b2305e..ee2b3bd89213 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -104,6 +104,7 @@ struct dev_pm_opp { unsigned int pstate; unsigned long *rates; unsigned int level; + enum dev_pm_opp_provider_type provider; struct dev_pm_opp_supply *supplies; struct dev_pm_opp_icc_bw *bandwidth; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 2c6f67736579..4c40199c7728 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -26,6 +26,11 @@ enum dev_pm_opp_event { OPP_EVENT_ADJUST_VOLTAGE, }; +enum dev_pm_opp_provider_type { + DEV_PM_OPP_TYPE_NONE = 0, + DEV_PM_OPP_TYPE_GENPD, +}; + /** * struct dev_pm_opp_supply - Power supply voltage/current values * @u_volt: Target voltage in microvolts corresponding to this OPP @@ -97,11 +102,13 @@ struct dev_pm_opp_config { * @level: The performance level for the OPP. * @freq: The clock rate in Hz for the OPP. * @u_volt: The voltage in uV for the OPP. + * @provider: The type of provider for the OPP. */ struct dev_pm_opp_data { unsigned int level; unsigned long freq; unsigned long u_volt; + enum dev_pm_opp_provider_type provider; }; #if defined(CONFIG_PM_OPP)
To allow a dynamically added OPP to be coupled with a specific OPP provider type, let's add a new enum variable in the struct dev_pm_opp_data. Moreover, let's add support for a DEV_PM_OPP_TYPE_GENPD type, corresponding to genpd's performance states support. More precisely, this allows a genpd provider to dynamically add OPPs when a device gets attached to it, that later can be used by a consumer driver when it needs to change the performance level for its corresponding device. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/opp/core.c | 19 +++++++++++++++++++ drivers/opp/opp.h | 1 + include/linux/pm_opp.h | 7 +++++++ 3 files changed, 27 insertions(+)