* Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 11:10 [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off Sudeep Holla
@ 2024-01-19 14:05 ` Pierre Gondois
2024-01-19 14:43 ` Sudeep Holla
2024-01-19 14:06 ` Cristian Marussi
2024-01-19 15:23 ` [PATCH v2] " Sudeep Holla
2 siblings, 1 reply; 8+ messages in thread
From: Pierre Gondois @ 2024-01-19 14:05 UTC (permalink / raw)
To: Sudeep Holla, linux-arm-kernel; +Cc: Cristian Marussi
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);
> }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 14:05 ` Pierre Gondois
@ 2024-01-19 14:43 ` Sudeep Holla
0 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2024-01-19 14:43 UTC (permalink / raw)
To: Pierre Gondois; +Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi
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.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 11:10 [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off Sudeep Holla
2024-01-19 14:05 ` Pierre Gondois
@ 2024-01-19 14:06 ` Cristian Marussi
2024-01-19 14:45 ` Sudeep Holla
2024-01-19 15:23 ` [PATCH v2] " Sudeep Holla
2 siblings, 1 reply; 8+ messages in thread
From: Cristian Marussi @ 2024-01-19 14:06 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-arm-kernel, Pierre Gondois
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 14:06 ` Cristian Marussi
@ 2024-01-19 14:45 ` Sudeep Holla
0 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2024-01-19 14:45 UTC (permalink / raw)
To: Cristian Marussi; +Cc: linux-arm-kernel, Sudeep Holla, Pierre Gondois
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 🙁.
> Other than that, LGTM.
>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>
Thanks!
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 11:10 [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off Sudeep Holla
2024-01-19 14:05 ` Pierre Gondois
2024-01-19 14:06 ` Cristian Marussi
@ 2024-01-19 15:23 ` Sudeep Holla
2024-01-19 15:26 ` Cristian Marussi
2024-02-22 7:50 ` Sudeep Holla
2 siblings, 2 replies; 8+ messages in thread
From: Sudeep Holla @ 2024-01-19 15:23 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Sudeep Holla, Pierre Gondois, Pierre Gondois, Cristian Marussi
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>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_scmi/perf.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
v1->v2:
- Added comment regarding the requirements on sustained_freq_khz
and sustained_perf_level obtained from the SCMI firmware
- Changed pr_warn to dev_warn
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 8ea2a7b3d35d..9fe123e1f3c7 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -270,15 +270,30 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
le32_to_cpu(attr->sustained_freq_khz);
dom_info->sustained_perf_level =
le32_to_cpu(attr->sustained_perf_level);
+ /*
+ * sustained_freq_khz = mult_factor * sustained_perf_level
+ * mult_factor must be non zero positive integer(not fraction)
+ */
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)
+ dev_warn(ph->dev,
+ "multiplier for domain %d rounded\n",
+ dom_info->id);
+ }
+ if (!dom_info->mult_factor)
+ dev_warn(ph->dev,
+ "Wrong sustained perf/frequency(domain %d)\n",
+ dom_info->id);
+
strscpy(dom_info->info.name, attr->name,
SCMI_SHORT_NAME_MAX_SIZE);
}
--
2.43.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 15:23 ` [PATCH v2] " Sudeep Holla
@ 2024-01-19 15:26 ` Cristian Marussi
2024-02-22 7:50 ` Sudeep Holla
1 sibling, 0 replies; 8+ messages in thread
From: Cristian Marussi @ 2024-01-19 15:26 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-arm-kernel, Pierre Gondois
On Fri, Jan 19, 2024 at 03:23:37PM +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.
>
> Suggested-by: Pierre Gondois <Pierre.Gondois@arm.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
LGTM.
Thanks,
Cristian
> drivers/firmware/arm_scmi/perf.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> v1->v2:
> - Added comment regarding the requirements on sustained_freq_khz
> and sustained_perf_level obtained from the SCMI firmware
> - Changed pr_warn to dev_warn
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 8ea2a7b3d35d..9fe123e1f3c7 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -270,15 +270,30 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
> le32_to_cpu(attr->sustained_freq_khz);
> dom_info->sustained_perf_level =
> le32_to_cpu(attr->sustained_perf_level);
> + /*
> + * sustained_freq_khz = mult_factor * sustained_perf_level
> + * mult_factor must be non zero positive integer(not fraction)
> + */
> 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)
> + dev_warn(ph->dev,
> + "multiplier for domain %d rounded\n",
> + dom_info->id);
> + }
> + if (!dom_info->mult_factor)
> + dev_warn(ph->dev,
> + "Wrong sustained perf/frequency(domain %d)\n",
> + dom_info->id);
> +
> strscpy(dom_info->info.name, attr->name,
> SCMI_SHORT_NAME_MAX_SIZE);
> }
> --
> 2.43.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
2024-01-19 15:23 ` [PATCH v2] " Sudeep Holla
2024-01-19 15:26 ` Cristian Marussi
@ 2024-02-22 7:50 ` Sudeep Holla
1 sibling, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2024-02-22 7:50 UTC (permalink / raw)
To: linux-arm-kernel, Sudeep Holla
Cc: Pierre Gondois, Pierre Gondois, Cristian Marussi
On Fri, 19 Jan 2024 15:23:37 +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.
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!
[1/1] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off
https://git.kernel.org/sudeep.holla/c/534224b958df
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread