linux-kernel - Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5AFDD747.7020509@codeaurora.org>
Date:   Thu, 17 May 2018 12:25:59 -0700
From:   Saravana Kannan <skannan@...eaurora.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
CC:     Taniya Das <tdas@...eaurora.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Amit Nischal <anischal@...eaurora.org>,
        devicetree@...r.kernel.org, amit.kucheria@...aro.org
Subject: Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

On 05/17/2018 03:14 AM, Viresh Kumar wrote:
> On 17-05-18, 15:00, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
>> for hanging the frequency of CPUs. The driver implements the cpufreq driver
>> interface for this firmware.
>>
>> Signed-off-by: Taniya Das <tdas@...eaurora.org>
>> ---
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c

>> +
>> +static int qcom_read_lut(struct platform_device *pdev,
>> +			struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	u32 data, src, lval, i, core_count, prev_cc = 0;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>> +		src = ((data & GENMASK(31, 30)) >> 30);
>> +		lval = (data & GENMASK(7, 0));
>> +		core_count = CORE_COUNT_VAL(data);
>
> Why do you need this here ? And why do below in case this doesn't
> match max-cores count ?

This is how we detect boost frequencies.

>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> +		c->table[i].driver_data = c->table[i].frequency;
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +				i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;

The FW might has some frequencies marked as "boost frequencies" when 
there are higher non-boost frequencies. So, we mark them as invalid.

>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].driver_data ==
>> +					c->table[i].driver_data
>> +					&& prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +				prev->frequency = prev->driver_data;
>> +			}
>> +
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +	}
>> +	c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> +	struct device_node *dev_phandle;
>> +	struct device *cpu_dev;
>> +	int cpu, i = 0, ret = -ENOENT;
>> +
>> +	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>
> TBH, I am not a great fan of the CPU phandle list you have created
> here. Lets see what Rob has to say on this.
>

Neither do we, but this is the only real way of mapping the logical CPU 
numbers to the real CPUs in HW that belong to the same freq domain. 
Because boot CPU is always going to be CPU0 if I'm not mistaken.

>> +	while (dev_phandle) {
>> +		for_each_possible_cpu(cpu) {
>> +			cpu_dev = get_cpu_device(cpu);
>> +			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
>> +				cpumask_set_cpu(cpu, m);
>> +				ret = 0;
>
> Maybe just remove this line ...
>
>> +				break;
>> +			}
>> +		}
>> +		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> +	}
>> +
>> +	return ret;


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project