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

[Sudeep Holla <sudeep.holla@arm.com>]

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