[PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Sudeep Holla]

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);
 	}
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Pierre Gondois]

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

Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Cristian Marussi]

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

Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Sudeep Holla]

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

Re: [PATCH] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Sudeep Holla]

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

[PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Sudeep Holla]

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

Re: [PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Cristian Marussi]

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

Re: [PATCH v2] firmware: arm_scmi: Warn if domain frequency multiplier is 0 or rounded off

[Sudeep Holla]

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