| Message ID | 20240108140118.1596-1-quic_sibis@quicinc.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Register and handle limits change notification | expand |
On 08-01-24, 19:31, Sibi Sankar wrote: > Add a new perf_opp_xlate interface to the existing perf_ops to translate > a given perf index to frequency. > > This can be used by the cpufreq driver and framework to determine the > throttled frequency from a given perf index and apply HW pressure > accordingly. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++ > include/linux/scmi_protocol.h | 3 +++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index c167bb5e3607..f26390924e1c 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -964,6 +964,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain > return 0; > } > > +static int scmi_perf_opp_xlate(const struct scmi_protocol_handle *ph, u32 domain, > + int idx, unsigned long *freq) > +{ > + struct perf_dom_info *dom; > + > + dom = scmi_perf_domain_lookup(ph, domain); > + if (IS_ERR(dom)) > + return PTR_ERR(dom); > + > + if (idx >= dom->opp_count) > + return -ERANGE; > + > + if (!dom->level_indexing_mode) > + *freq = dom->opp[idx].perf * dom->mult_factor; > + else > + *freq = dom->opp[idx].indicative_freq * dom->mult_factor; > + > + return 0; > +} > + > static const struct scmi_perf_proto_ops perf_proto_ops = { > .num_domains_get = scmi_perf_num_domains_get, > .info_get = scmi_perf_info_get, > @@ -979,6 +999,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = { > .fast_switch_possible = scmi_fast_switch_possible, > .power_scale_get = scmi_power_scale_get, > .perf_notify_support = scmi_notify_support, > + .perf_opp_xlate = scmi_perf_opp_xlate, The use of "opp" here is a bit confusing as this doesn't have anything to do with the OPP framework and you are only getting the frequency out of it after all.
On 1/10/24 12:59, Viresh Kumar wrote: > On 08-01-24, 19:31, Sibi Sankar wrote: >> Add a new perf_opp_xlate interface to the existing perf_ops to translate >> a given perf index to frequency. >> Hey Viresh, Thanks for taking time to review the series! >> This can be used by the cpufreq driver and framework to determine the >> throttled frequency from a given perf index and apply HW pressure >> accordingly. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++ >> include/linux/scmi_protocol.h | 3 +++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c >> index c167bb5e3607..f26390924e1c 100644 >> --- a/drivers/firmware/arm_scmi/perf.c >> +++ b/drivers/firmware/arm_scmi/perf.c >> @@ -964,6 +964,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain >> return 0; >> } >> >> +static int scmi_perf_opp_xlate(const struct scmi_protocol_handle *ph, u32 domain, >> + int idx, unsigned long *freq) >> +{ >> + struct perf_dom_info *dom; >> + >> + dom = scmi_perf_domain_lookup(ph, domain); >> + if (IS_ERR(dom)) >> + return PTR_ERR(dom); >> + >> + if (idx >= dom->opp_count) >> + return -ERANGE; >> + >> + if (!dom->level_indexing_mode) >> + *freq = dom->opp[idx].perf * dom->mult_factor; >> + else >> + *freq = dom->opp[idx].indicative_freq * dom->mult_factor; >> + >> + return 0; >> +} >> + >> static const struct scmi_perf_proto_ops perf_proto_ops = { >> .num_domains_get = scmi_perf_num_domains_get, >> .info_get = scmi_perf_info_get, >> @@ -979,6 +999,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = { >> .fast_switch_possible = scmi_fast_switch_possible, >> .power_scale_get = scmi_power_scale_get, >> .perf_notify_support = scmi_notify_support, >> + .perf_opp_xlate = scmi_perf_opp_xlate, > > The use of "opp" here is a bit confusing as this doesn't have anything to do > with the OPP framework and you are only getting the frequency out of it after > all. Sure will re-name it. -Sibi >
On Mon, Jan 08, 2024 at 07:31:15PM +0530, Sibi Sankar wrote: > This series registers for scmi limits change notifications and adds > perf_notify_support/perf_opp_xlate interfaces which are used by the > scmi cpufreq driver to determine the throttled frequency and apply HW > pressure. > Hi, a few initial remarks from the mere SCMI standpoint. Unlinke most SCMI protocols that expose domains info bits via an *info_get protocol operation, PERF does no do this since (till now) there wasn't a compelling reason (i.e. users) Ulf recently in his GenPD/SCMI series recently started exposing something and now you need to expose even more, adding also a new xlate ops. For the sake of simplicity, I think that we could now expose straight away the whole perf_domain_info and embedded structs via the usual *info_get. After having done that, you can just drop your patch 1 and 2 since you can access the needed info from the cpufreq_driver right away. Having said, I have already such patch ready (for my internal testing), I wll post it by the end of week after a minor cleanup, if you can bear with me. Thoughts ? Thanks, Cristian
On 1/17/24 15:11, Cristian Marussi wrote: > On Mon, Jan 08, 2024 at 07:31:15PM +0530, Sibi Sankar wrote: >> This series registers for scmi limits change notifications and adds >> perf_notify_support/perf_opp_xlate interfaces which are used by the >> scmi cpufreq driver to determine the throttled frequency and apply HW >> pressure. >> > > Hi, > > a few initial remarks from the mere SCMI standpoint. > > Unlinke most SCMI protocols that expose domains info bits via an > *info_get protocol operation, PERF does no do this since (till now) there > wasn't a compelling reason (i.e. users) > > Ulf recently in his GenPD/SCMI series recently started exposing something > and now you need to expose even more, adding also a new xlate ops. > > For the sake of simplicity, I think that we could now expose straight > away the whole perf_domain_info and embedded structs via the usual *info_get. > > After having done that, you can just drop your patch 1 and 2 since you > can access the needed info from the cpufreq_driver right away. > > Having said, I have already such patch ready (for my internal testing), I > wll post it by the end of week after a minor cleanup, if you can bear with me. > > Thoughts ? Ack, just from the naming I initially thought info_get would include everything but it just exposed minimal info. We certainly don't want to keep adding very similar ops just to expose more such info. I'll re-send the remainder of the series after you are done with your patches. Thanks. -Sibi > > Thanks, > Cristian