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

[Jie Zhan]

On 20/06/2025 21:10, Jonathan Cameron wrote:
> On Thu, 19 Jun 2025 23:14:56 +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 more things inline that I missed in earlier review.
> 
> The devm_mutex one is a definite thing to fix as having it
> where it is will make it far to easy to add bugs.
> 
> The other stuff is a nice to have.
> 
> So with at least the devm_mutex() call moved
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron at huawei.com>
> 
> 
Hi Jonathan,

Thanks for reviewing again.
Will update the next version soon.

Jie

>> diff --git a/drivers/devfreq/hisi_uncore_freq.c b/drivers/devfreq/hisi_uncore_freq.c
>> new file mode 100644
>> index 000000000000..e19678692c16
>> --- /dev/null
>> +++ b/drivers/devfreq/hisi_uncore_freq.c
>> @@ -0,0 +1,664 @@
> 
>> +static int hisi_uncore_request_pcc_chan(struct hisi_uncore_freq *uncore)
>> +{
>> +	struct device *dev = uncore->dev;
>> +	struct pcc_mbox_chan *pcc_chan;
>> +	int rc;
>> +
>> +	uncore->cl = (struct mbox_client) {
>> +		.dev = dev,
>> +		.tx_block = false,
>> +		.knows_txdone = true,
>> +	};
>> +
>> +	pcc_chan = pcc_mbox_request_channel(&uncore->cl, uncore->chan_id);
> 
> I'm not particularly keen on the repeats of pcc_mbox_free_channel() in each of
> the error paths.  Either use a goto and clean it up at the end or
> DEFINE_FREE(pcc_mbox_chan, struct pcc_mbox_chan *, if (_T) pcc_mbox_free_channel(_T));
> 
> then here
> 	struct pcc_mbox_chan __free(pcc_mbox_chan) *pcc_chan =
> 		pcc_mbox_request_channel(&uncore->cl, uncore->chan_id);
> 
> remembering to do 
> 	uncor->chan = no_free_ptr(pcc_chan);
> where you currently set it below.
> 
> The DEFINE_FREE() might prove useful in other drivers.  kunpeng_hccs
> for instance could be simplified with this.  Various other candidates
> though not sure how keen on cleanup.h magic those areas of the kernel are.
> 
> 
Yeah, good suggestion for cleanup, but the DEFINE_FREE() stuff should go
into acpi/pcc.h.

I think it's better to make it a new patchset later, including the new
DEFINE_FREE() for pcc_mbox_free_channel() and other corresponding driver
changes, and send it to the ACPI/PCC folks.  Let's keep it unchanged for
this driver patch at this stage.
>> +	if (IS_ERR(pcc_chan))
>> +		return dev_err_probe(dev, PTR_ERR(pcc_chan),
>> +			"Failed to request PCC channel %u\n", uncore->chan_id);
>> +
>> +	if (!pcc_chan->shmem_base_addr) {
>> +		pcc_mbox_free_channel(pcc_chan);
>> +		return dev_err_probe(dev, -EINVAL,
>> +			"Invalid PCC shared memory address\n");
>> +	}
>> +
>> +	if (pcc_chan->shmem_size < sizeof(struct hisi_uncore_pcc_shmem)) {
>> +		pcc_mbox_free_channel(pcc_chan);
>> +		return dev_err_probe(dev, -EINVAL,
>> +			"Invalid PCC shared memory size (%lluB)\n",
>> +			pcc_chan->shmem_size);
>> +	}
>> +
>> +	rc = devm_mutex_init(dev, &uncore->pcc_lock);
> 
> This is oddly placed.  Drop this out of this function and do it at least one layer
> up.
> 
> Having it here leaves the risk of future error cases being added after this
> where the devm cleanup will happen out of order as a result.
> 
Thanks, nice spot.
hisi_uncore_init_pcc_chan() is a suitable place.
>> +	if (rc) {
>> +		pcc_mbox_free_channel(pcc_chan);
>> +		return rc;
>> +	}
>> +
>> +	uncore->pchan = pcc_chan;
>> +
>> +	return 0;
>> +}