[PATCH v4 2/2] PM / devfreq: Add HiSilicon uncore frequency scaling driver

[Jie Zhan]

On 16/06/2025 17:35, Jonathan Cameron wrote:
> On Fri, 30 May 2025 16:17:22 +0800
> Jie Zhan <zhanjie9 at hisilicon.com> wrote:
> 
>> Add the HiSilicon uncore frequency scaling driver for Kunpeng SoCs based on
>> the devfreq framework.  The uncore domain contains shared computing
>> resources, including system interconnects and L3 cache.  The uncore
>> frequency significantly impacts the system-wide performance as well as
>> power consumption.  This driver adds support for runtime management of
>> uncore frequency from kernel and userspace.  The main function includes
>> setting and getting frequencies, changing frequency scaling policies, and
>> querying the list of CPUs whose performance is significantly related to
>> this uncore frequency domain, etc.  The driver communicates with a platform
>> controller through an ACPI PCC mailbox to take the actual actions of
>> frequency scaling.
>>
>> Co-developed-by: Lifeng Zheng <zhenglifeng1 at huawei.com>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1 at huawei.com>
>> Signed-off-by: Jie Zhan <zhanjie9 at hisilicon.com>
> Hi Zhanjie,
> 
> A few comments inline.  In general nice and clean.
> 
> I think only one that really needs a change it the one around the
> CPU association firmware handling.
> 
> Jonathan
> 
Thanks again!

All accepted, except for the cleanup of putting devm_add_action_or_reset()
before opp_add().

See inline replies below.

Jie

> 
>> ---
>>  Documentation/ABI/testing/sysfs-class-devfreq |   9 +
>>  drivers/devfreq/Kconfig                       |  11 +
>>  drivers/devfreq/Makefile                      |   1 +
>>  drivers/devfreq/hisi_uncore_freq.c            | 656 ++++++++++++++++++
>>  4 files changed, 677 insertions(+)
>>  create mode 100644 drivers/devfreq/hisi_uncore_freq.c
>>

>> +		rc = dev_pm_opp_add(dev, freq_mhz * HZ_PER_MHZ, 1000000);
>> +		if (rc) {
>> +			dev_pm_opp_remove_all_dynamic(dev);
>> +			return dev_err_probe(dev, rc,
>> +				"Add OPP %lu failed\n", freq_mhz);
>> +		}
>> +	}
>> +
>> +	return devm_add_action_or_reset(dev, devm_hisi_uncore_remove_opp, uncore);
> Hmm. I'm normally a fan of registering these after the calls, but this is one
> of the rare cases where pushing it before (with a comment) cleans up the code.
> 
> If you do that, then all the error cases in the loop just need to return and not
> call the cleanup manually.
I'd prefer to keep the current order so as to keep logic and style
consistent across places where devm_add_action is used.  It would be easier
for people to follow.
> 
...
>> +static int hisi_uncore_mark_related_cpus(struct hisi_uncore_freq *uncore,
>> +				 char *property, int (*get_topo_id)(int cpu),
>> +				 const struct cpumask *(*get_cpumask)(int cpu))
>> +{
>> +	unsigned int i, cpu;
>> +	size_t len;
>> +	int rc;
>> +
>> +	rc = device_property_count_u32(uncore->dev, property);
>> +	if (rc < 0)
>> +		return rc;
> Most of the errors here don't reflect it not being found and are things
> were we should probably fail the driver probe (so someone can fix whatever
> is wrong with the firmware.)  I think only -EINVAL means not here
> (technically arguments are not valid)
> 

Thanks for spotting this.

Had a dig.

In the case here:
rc > 0, success, rc is the number of elements in this property.
rc == 0, property found but no elements in it, the firmware might have a
property without values.
rc == -EINVAL, property not found, or invalid arguments received in any of
the subroutine, e.g. NULL pointer.
rc < 0 && rc != -EINVAL, firmware probably broken.

I think we can take 0 and -EINVAL as not found, and try the other property
name.  If rc == 0, the firmware might just have some redundant code, not a
big issue.  If rc == -EINVAL, it's likely that the property is not found
unless some data go wrong and make arguments invalid.

Other negative error codes should fail the driver probe.

The device_property_read_u32_array() below is similar.
I'll put this in comments and update hisi_uncore_mark_related_cpus_wrap().
This function doesn't need to change.

>> +	if (rc == 0)
>> +		return -EINVAL;
>> +
>> +	len = rc;
>> +	u32 *num __free(kfree) = kcalloc(len, sizeof(*num), GFP_KERNEL);
>> +	if (!num)
>> +		return -ENOMEM;
> 

...

>> +static int hisi_uncore_devfreq_register(struct hisi_uncore_freq *uncore)
>> +{
>> +	struct devfreq_dev_profile *profile;
>> +	struct device *dev = uncore->dev;
>> +	unsigned long freq;
>> +	u32 data;
>> +	int rc;
>> +
>> +	rc = hisi_uncore_get_cur_freq(dev, &freq);
> One for another day:
> Whilst we do indeed need to do this, it seems like a small optimization to
> devfreq would be to check for initial_freq == 0 and if it is try get_cur_freq()
> after registration. Mind you I checked and this only seems to apply to
> the imx drivers and this one.
> 
Yeah, understood.
Let's check and improve that later.