Re: [PATCH 2/2] cpufreq: qcom-hw: Ensure only freq-domain regs are counted in num_domains

[Konrad Dybcio]

On 13.01.2023 20:41, Rob Herring wrote:
> On Thu, Jan 12, 2023 at 04:41:50PM +0100, Konrad Dybcio wrote:
>>
>>
>> On 12.01.2023 16:37, Bjorn Andersson wrote:
>>> On Wed, Jan 11, 2023 at 09:51:25PM +0100, Konrad Dybcio wrote:
>>>> In preparation for CPRh-aware OSM programming, change the probe
>>>> function so that we determine the number of frequency domains by
>>>> counting the number of reg-names entries that begin with
>>>> "freq-domain", as the aforementioned changes require introduction
>>>> of non-freq-domain register spaces.
>>>>
>>>
>>> Requiring reg-names would break backwards compatibility with at least
>>> sc7280 and sm6115.
>> Ouch, you're correct..
>>
>> Does checking for reg-names and applying the code flow proposed in this
>> patch if found and the existing one if not sound good?
> 
> Why support 2 ways?
Targets that are supported by the current revision of this driver
(which only specify frequency-domain-N MMIO spaces as reg
entries) assume that ARRAY_SIZE(reg) == the number of frequency
domains. These usually range from 1 to 3.

We can either hardcode the number of frequency domains on targets
that require more, different register spaces (for manual hardware
programming, which also happens on currently supported hardware,
just that the secure firmware does it for us.. see [1])
or check with reg-names. Requiring reg-names would break backwards
compatibility with older DTs for at least two SoCs.


> 
> 
>> Konrad
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> Fixes: 1a6a8b0080b0 ("cpufreq: qcom-hw: Fix reading "reg" with address/size-cells != 2")
>>>> Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe")
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>>>> ---
>>>>  drivers/cpufreq/qcom-cpufreq-hw.c | 34 ++++++++++++++++++++++---------
>>>>  1 file changed, 24 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>> index 9505a812d6a1..89d5ed267399 100644
>>>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>> @@ -651,8 +651,9 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>>>>  	struct device *dev = &pdev->dev;
>>>>  	struct device_node *soc_node;
>>>>  	struct device *cpu_dev;
>>>> +	const char *reg_name;
>>>>  	struct clk *clk;
>>>> -	int ret, i, num_domains, reg_sz;
>>>> +	int ret, i, num_reg_names, num_domains = 0;
>>>>  
>>>>  	clk = clk_get(dev, "xo");
>>>>  	if (IS_ERR(clk))
>>>> @@ -684,19 +685,32 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>>>>  	if (!soc_node)
>>>>  		return -EINVAL;
>>>>  
>>>> -	ret = of_property_read_u32(soc_node, "#address-cells", &reg_sz);
>>>> -	if (ret)
>>>> +	num_reg_names = of_property_count_strings(dev->of_node, "reg-names");
>>>> +	if (num_reg_names <= 0) {
>>>> +		ret = num_reg_names ? num_reg_names : -ENODATA;
>>>>  		goto of_exit;
>>>> +	}
>>>>  
>>>> -	ret = of_property_read_u32(soc_node, "#size-cells", &i);
>>>> -	if (ret)
>>>> -		goto of_exit;
>>>> +	for (i = 0; i < num_reg_names; i++) {
>>>> +		ret = of_property_read_string_index(dev->of_node, "reg-names", i, &reg_name);
>>>> +		if (ret < 0)
>>>> +			goto of_exit;
>>>>  
>>>> -	reg_sz += i;
>>>> +		/*
>>>> +		 * Check if the i-th reg is a freq-domain base, no need to add 1
>>>> +		 * more byte for idx, as sizeof counts \0 whereas strlen does not.
>>>> +		 */
>>>> +		if (strlen(reg_name) == sizeof("freq-domain")) {
>>>> +			/* Check if this reg-name begins with "freq-domain" */
>>>> +			if (!strncmp(reg_name, "freq-domain", sizeof("freq-domain") - 1))
>>>> +				num_domains++;
>>>> +		}
>>>> +	}
>>>>  
>>>> -	num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz);
> 
> This code was not great to begin with. Any code parsing 'reg' on it's 
> own is suspect IMO. It's a standard property and all parsing of it 
> should be in drivers/of/address.c. (Yes, I know there are other cases.)
> 
> The reg entries are already available as platform_device resources? Why 
> don't you use that? There's also of_address_count(), but I prefer if 
> there's a platform device equivalent like we have for interrupts.
Hm.. I knew this was suspiciously bare-dt-operation, but never quite
connected the dots.. perhaps that's a good idea to pursue..

Konrad

[1] https://patchwork.kernel.org/project/linux-pm/patch/20210701105730.322718-7-angelogioacchino.delregno@xxxxxxxxxxxxxx/
> 
> Rob