| Message ID | 20240119111033.3011853-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off | expand |
Hello Sudeep, Thanks for the patch. Just to add a note, it means that the firmware must have freq and level values such as: - freq = K * level - K >= 1 - K being an integer If K is not an integer, the bigger K is, the lower the imprecision in the available frequencies is. Having imprecise frequency values did not impact the frequency changes when I tried it. Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> On 1/19/24 12:10, Sudeep Holla wrote: > When (sustained_freq_khz * 1000) is less than sustained_perf_level, the > multiplier will be less than 1 and hence rounded down as 0. Similarly if > it is not multiple of sustained_perf_level the dom_info->mult_factor will > contain rounded down value and will end up impacting all the frequency > calculations done using it. > > Add warning if and when the domain frequency multiplier is 0 or rounded > down so that it gives a clue to get the firmware tables fixed. > > Suggested-by: Pierre Gondois <Pierre.Gondois@arm.com> > Cc: Cristian Marussi <cristian.marussi@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/perf.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 8ea2a7b3d35d..5a7358e89d6f 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -272,13 +272,21 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, > le32_to_cpu(attr->sustained_perf_level); > if (!dom_info->sustained_freq_khz || > !dom_info->sustained_perf_level || > - dom_info->level_indexing_mode) > + dom_info->level_indexing_mode) { > /* CPUFreq converts to kHz, hence default 1000 */ > dom_info->mult_factor = 1000; > - else > + } else { > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) > / dom_info->sustained_perf_level; > + if ((dom_info->sustained_freq_khz * 1000UL) % > + dom_info->sustained_perf_level) > + pr_warn("mult_factor of domain %d is rounded\n", > + dom_info->id); > + } > + if (!dom_info->mult_factor) > + pr_warn("Incorrect sustained perf level or freq kHz\n"); > + > strscpy(dom_info->info.name, attr->name, > SCMI_SHORT_NAME_MAX_SIZE); > }
On Fri, Jan 19, 2024 at 11:10:33AM +0000, Sudeep Holla wrote: > When (sustained_freq_khz * 1000) is less than sustained_perf_level, the > multiplier will be less than 1 and hence rounded down as 0. Similarly if > it is not multiple of sustained_perf_level the dom_info->mult_factor will > contain rounded down value and will end up impacting all the frequency > calculations done using it. > > Add warning if and when the domain frequency multiplier is 0 or rounded > down so that it gives a clue to get the firmware tables fixed. > Hi, just one small nitpick down below... > Suggested-by: Pierre Gondois <Pierre.Gondois@arm.com> > Cc: Cristian Marussi <cristian.marussi@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/perf.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 8ea2a7b3d35d..5a7358e89d6f 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -272,13 +272,21 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, > le32_to_cpu(attr->sustained_perf_level); > if (!dom_info->sustained_freq_khz || > !dom_info->sustained_perf_level || > - dom_info->level_indexing_mode) > + dom_info->level_indexing_mode) { > /* CPUFreq converts to kHz, hence default 1000 */ > dom_info->mult_factor = 1000; > - else > + } else { > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) > / dom_info->sustained_perf_level; > + if ((dom_info->sustained_freq_khz * 1000UL) % > + dom_info->sustained_perf_level) > + pr_warn("mult_factor of domain %d is rounded\n", > + dom_info->id); ...here you can use dev_warn(ph->dev, ...) if you want. Other than that, LGTM. Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
On Fri, Jan 19, 2024 at 03:05:09PM +0100, Pierre Gondois wrote: > Hello Sudeep, > Thanks for the patch. Just to add a note, it means that the firmware must have > freq and level values such as: > - freq = K * level > - K >= 1 > - K being an integer > Correct, if it helps for future references, I can add the info as a comment in the code. > If K is not an integer, the bigger K is, the lower the imprecision in the > available frequencies is. Having imprecise frequency values did not impact > the frequency changes when I tried it. Agreed. But I don't want to make any assumption on the values to throw this warning. We can silence it to debug in the future if it gets annoying on some real systems where it doesn't matter much. I just want to keep it to give f/w a change to correct/adjust it if they can. > > Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> > Thanks.
On Fri, Jan 19, 2024 at 02:06:56PM +0000, Cristian Marussi wrote: > On Fri, Jan 19, 2024 at 11:10:33AM +0000, Sudeep Holla wrote: > > When (sustained_freq_khz * 1000) is less than sustained_perf_level, the > > multiplier will be less than 1 and hence rounded down as 0. Similarly if > > it is not multiple of sustained_perf_level the dom_info->mult_factor will > > contain rounded down value and will end up impacting all the frequency > > calculations done using it. > > > > Add warning if and when the domain frequency multiplier is 0 or rounded > > down so that it gives a clue to get the firmware tables fixed. > > > > Hi, > > just one small nitpick down below... > > > Suggested-by: Pierre Gondois <Pierre.Gondois@arm.com> > > Cc: Cristian Marussi <cristian.marussi@arm.com> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > drivers/firmware/arm_scmi/perf.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > > index 8ea2a7b3d35d..5a7358e89d6f 100644 > > --- a/drivers/firmware/arm_scmi/perf.c > > +++ b/drivers/firmware/arm_scmi/perf.c > > @@ -272,13 +272,21 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, > > le32_to_cpu(attr->sustained_perf_level); > > if (!dom_info->sustained_freq_khz || > > !dom_info->sustained_perf_level || > > - dom_info->level_indexing_mode) > > + dom_info->level_indexing_mode) { > > /* CPUFreq converts to kHz, hence default 1000 */ > > dom_info->mult_factor = 1000; > > - else > > + } else { > > dom_info->mult_factor = > > (dom_info->sustained_freq_khz * 1000UL) > > / dom_info->sustained_perf_level; > > + if ((dom_info->sustained_freq_khz * 1000UL) % > > + dom_info->sustained_perf_level) > > + pr_warn("mult_factor of domain %d is rounded\n", > > + dom_info->id); > > ...here you can use dev_warn(ph->dev, ...) if you want. > Good point, I didn't even check the logging elsewhere in the file
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index 8ea2a7b3d35d..5a7358e89d6f 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -272,13 +272,21 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, le32_to_cpu(attr->sustained_perf_level); if (!dom_info->sustained_freq_khz || !dom_info->sustained_perf_level || - dom_info->level_indexing_mode) + dom_info->level_indexing_mode) { /* CPUFreq converts to kHz, hence default 1000 */ dom_info->mult_factor = 1000; - else + } else { dom_info->mult_factor = (dom_info->sustained_freq_khz * 1000UL) / dom_info->sustained_perf_level; + if ((dom_info->sustained_freq_khz * 1000UL) % + dom_info->sustained_perf_level) + pr_warn("mult_factor of domain %d is rounded\n", + dom_info->id); + } + if (!dom_info->mult_factor) + pr_warn("Incorrect sustained perf level or freq kHz\n"); + strscpy(dom_info->info.name, attr->name, SCMI_SHORT_NAME_MAX_SIZE); }
When (sustained_freq_khz * 1000) is less than sustained_perf_level, the multiplier will be less than 1 and hence rounded down as 0. Similarly if it is not multiple of sustained_perf_level the dom_info->mult_factor will contain rounded down value and will end up impacting all the frequency calculations done using it. Add warning if and when the domain frequency multiplier is 0 or rounded down so that it gives a clue to get the firmware tables fixed. Suggested-by: Pierre Gondois <Pierre.Gondois@arm.com> Cc: Cristian Marussi <cristian.marussi@arm.com> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/perf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)