[PATCH v2] cpufreq: Add test cases to validate the frequency and pstate table

[Shilpasri G Bhat]

Hi Jeremy,

On 05/03/2016 01:06 PM, Jeremy Kerr wrote:
> Hi Shilparsi,
> 
> This patch is looking good - but still a couple of minor changes that
> I'd suggest:

Thanks for the review.

> 
>> +static int cpufreq_test_frequency_limits(fwts_framework *fw)
> 
> Just curious: do we fail both of those tests on the recent turbo-mode
> OPAL bug? Does Linux still parse the invalid range of pstates from the
> DT and export them to /sys/?

We will fail the second test 'cpufreq_test_pstate_limits' in the turbo-mode OPAL
bug. The first test 'cpufreq_test_frequency_limits' will pass as min and max
frequency are computed by cpufreq core from the frequency table which is passed
by the driver (by parsing ibm,pstate-frequencies-mhz property).

/sys/devices/system/cpu/cpuX/cpufreq/cpuinfo_max_freq and
/sys/devices/system/cpu/cpuX/cpufreq/cpuinfo_min_freq will point to min and max
entries in 'ibm,pstate-frequencies-mhz' property.

Yes Linux directly uses the pstates from DT and exports to /sys/. Currently we
are clipping out the invalid range of pstates in OPAL.

The second test will fail in turbo-mode OPAL bug as the min and max pstate are
same as what is provided by OCC. (which is exported as DT properties in
ibm,pstate-max and ibm,pstate-min)

> 
>> +#if FWTS_HAS_DEVICETREE
>> +static int cpufreq_test_pstate_limits(fwts_framework *fw)
> 
> It's a close call on whether we put this in the cpufreq tests, or the
> device tree tests. The latter would mean that we don't start adding
> conditional compilation into the cpufreq code, so I think I'd prefer
> that (but that's just my opinion, the FWTS folks may disagree!)
> 

Okay.

>> +{
>> +	const char *power_mgt_path = "/ibm,opal/power-mgt/";
>> +	int pstate_min, pstate_max, pstates[MAX_FREQS], nr_pstates;
>> +	bool ok = true;
>> +	int  offset, len, ret, i;
>> +
>> +	if (!fw->fdt) {
>> +		fwts_skipped(fw, "Device tree not found");
>> +		return FWTS_SKIP;
>> +	}
>> +
>> +	offset = fdt_path_offset(fw->fdt, power_mgt_path);
>> +	if (offset < 0) {
>> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +			    "power management node is missing");
>> +		return FWTS_ERROR;
>> +	}
> 
> This isn't necessarily an error on all device tree platforms - consider
> non-OPAL POWER machines.
> 
> You might want to skip this test on the condition:
> 
> 	if (fwts_firmware_detect() != FWTS_FIRMWARE_OPAL)
> 
> [Also, do this at the beginning of the test, so that non-OPAL machines
> don't print out messages about no device tree.]

Yes agree. Will post the next version with this change.
> 
> Looks good though - I like the new device-tree helpers too.
> 

Thanks and Regards,
Shilpa