[Skiboot] [PATCH 1/2] OPAL/FWTS: Power Management DT Validation tests

ppaidipe ppaidipe at linux.vnet.ibm.com
Sat Apr 8 17:54:20 AEST 2017


Hi Vaidy

Thanks for the review.


On 2017-04-07 23:31, Vaidyanathan Srinivasan wrote:
> * Pridhiviraj Paidipeddi <ppaidipe at linux.vnet.ibm.com> [2017-04-06 
> 19:03:52]:
> 
>> Sending here to get some initial review.
>> Will send it to fwts upstream once patches are looking good.
>> 
>> This patch contains testcases for below Power Processor
>> energey management subsystems.
>> 	a. cpuidle
>> 	b. cpufreq
>> 
>> These testcases validate the device tree properties for these two
>> subsystems which are got exposed to linux from the system
>> firmware(OPAL).
>> 
>> This patch is enhanced based on the initial patch developed by shilpa
>> for pstates.
>> https://lists.ubuntu.com/archives/fwts-devel/2016-May/007874.html
>> 
>> Added cpuidle states DT Validation tests.
> 
> Hi Pridhivi,
> 
> You have covered good details in this test across PStates and cpuidle.
> 
> 
>> Signed-off-by: Pridhiviraj Paidipeddi <ppaidipe at linux.vnet.ibm.com>
>> ---
>>  src/Makefile.am                   |   1 +
>>  src/lib/include/fwts_cpu.h        |  18 ++
>>  src/lib/include/fwts_devicetree.h |  29 ++++
>>  src/lib/src/fwts_devicetree.c     | 152 ++++++++++++++++
>>  src/opal/power_mgmt_info.c        | 357 
>> ++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 557 insertions(+)
>>  create mode 100644 src/opal/power_mgmt_info.c
>> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index c1eb285..e833554 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -147,6 +147,7 @@ fwts_SOURCES = main.c 				\
>>  	kernel/version/version.c 		\
>>  	opal/mtd_info.c				\
>>  	opal/prd_info.c				\
>> +	opal/power_mgmt_info.c			\
>>  	pci/aspm/aspm.c 			\
>>  	pci/crs/crs.c 				\
>>  	pci/maxreadreq/maxreadreq.c 		\
>> diff --git a/src/lib/include/fwts_cpu.h b/src/lib/include/fwts_cpu.h
>> index 9ab2ccf..42cca25 100644
>> --- a/src/lib/include/fwts_cpu.h
>> +++ b/src/lib/include/fwts_cpu.h
>> @@ -33,6 +33,24 @@ typedef struct cpuinfo_x86 {
>>  	char *flags;		/* String containing flags */
>>  } fwts_cpuinfo_x86;
>> 
>> +/* PowerPC Processor specific bits */
>> +/* PVR definitions */
>> +#define PVR_TYPE_P7     0x003f
>> +#define PVR_TYPE_P7P    0x004a
>> +#define PVR_TYPE_P8E    0x004b /* Murano */
>> +#define PVR_TYPE_P8     0x004d /* Venice */
>> +#define PVR_TYPE_P8NVL  0x004c /* Naples */
>> +#define PVR_TYPE_P9     0x004e
>> +
>> +/* Processor generation */
>> +enum proc_gen {
>> +	proc_gen_unknown,
>> +	proc_gen_p7,            /* P7 and P7+ */
>> +	proc_gen_p8,
>> +	proc_gen_p9,
>> +};
>> +extern enum proc_gen proc_gen;
>> +
>>  typedef struct cpu_benchmark_result {
>>  	bool		cycles_valid;
>>  	uint64_t	loops;
>> diff --git a/src/lib/include/fwts_devicetree.h 
>> b/src/lib/include/fwts_devicetree.h
>> index 662f6ec..8ad867a 100644
>> --- a/src/lib/include/fwts_devicetree.h
>> +++ b/src/lib/include/fwts_devicetree.h
>> @@ -42,6 +42,15 @@
>>  #if FWTS_HAS_DEVICETREE
>> 
>>  int fwts_devicetree_read(fwts_framework *fwts);
>> +int fwts_dt_property_read_u32(void *fdt, int offset, const char 
>> *pname,
>> +			      int *value);
>> +int fwts_dt_property_read_u32_arr(void *fdt, int offset, const char 
>> *pname,
>> +				  int *value, int *len);
>> +int fwts_dt_stringlist_count(fwts_framework *fw, const void *fdt,
>> +				 int nodeoffset, const char *property);
>> +int validate_dt_prop_sizes(fwts_framework *fw,
>> +                    const char *prop1, int prop1_len,
>> +                    const char *prop2, int prop2_len);
>> 
>>  #else /* !FWTS_HAS_DEVICETREE */
>>  static inline int fwts_devicetree_read(fwts_framework *fwts)
>> @@ -50,6 +59,24 @@ static inline int 
>> fwts_devicetree_read(fwts_framework *fwts)
>> 
>>  	return FWTS_OK;
>>  }
>> +
>> +static inline int fwts_dt_property_read_u32(void *fdt 
>> __attribute__((unused)),
>> +			int offset __attribute__((unused)),
>> +			const char *pname __attribute__((unused)),
>> +			int *value __attribute__((unused)))
>> +{
>> +	return FWTS_OK;
>> +}
>> +
>> +static inline int fwts_dt_property_read_u32_arr(void *fdt
>> +			__attribute__((unused)),
>> +			int offset __attribute__((unused)),
>> +			const char *pname __attribute__((unused)),
>> +			int *value __attribute__((unused)),
>> +			int *len __attribute__((unused)))
>> +{
>> +	return FWTS_OK;
>> +}
>>  #endif
>> 
>>  bool check_status_property_okay(fwts_framework *fw,
>> @@ -60,4 +87,6 @@ int check_property_printable(fwts_framework *fw,
>> 
>>  char *hidewhitespace(char *name);
>> 
>> +int get_proc_gen(fwts_framework *fw);
>> +
>>  #endif
>> diff --git a/src/lib/src/fwts_devicetree.c 
>> b/src/lib/src/fwts_devicetree.c
>> index bf5686a..f86cd13 100644
>> --- a/src/lib/src/fwts_devicetree.c
>> +++ b/src/lib/src/fwts_devicetree.c
>> @@ -26,6 +26,8 @@
>> 
>>  #include <libfdt.h>
>> 
>> +enum proc_gen proc_gen;
>> +
>>  int fwts_devicetree_read(fwts_framework *fwts)
>>  {
>>  	char *command, *data = NULL;
>> @@ -171,3 +173,153 @@ char *hidewhitespace(char *name)
>>  	return name;
>> 
>>  }
>> +
>> +int fwts_dt_property_read_u32(void *fdt, int offset, const char 
>> *pname,
>> +			int *value)
>> +{
>> +	int len;
>> +	const int *buf;
>> +
>> +	buf = fdt_getprop(fdt, offset, pname, &len);
>> +	if (buf == NULL) {
>> +		*value = len;
>> +		return FWTS_ERROR;
>> +	}
>> +	*value = be32toh(*buf);
>> +	return FWTS_OK;
>> +}
>> +
>> +int fwts_dt_property_read_u32_arr(void *fdt, int offset, const char 
>> *pname,
>> +				  int *value, int *len)
>> +{
>> +	int i;
>> +	const int *buf;
>> +
>> +	buf = fdt_getprop(fdt, offset, pname, len);
>> +	if (buf == NULL) {
>> +		*value = *len;
>> +		return FWTS_ERROR;
>> +	}
>> +
>> +	*len = *len / sizeof(int);
>> +	for (i = 0; i < *len; i++)
>> +		value[i] = be32toh(buf[i]);
> 
> Can you explain what you expect in value from caller?  Is it one u32
> or the full array?
> 
> I think on failure sending *value = *len to caller will confuse the
> caller.
> 
> fdt_getprop will return len in bytes or number of u32?
> 
> 

fdt_getprop stores the both length and return error code in len based on
property existence.

if property exists:
          it returns pointer to the property's value
          if len is non-NULL, *len contains the length of the property 
value (>=0) in bytes
else
          it returns NULL, on error
          if len is non-NULL, *len contains an error code (<0)

so similar way

fwts_dt_property_read_u32_arr
if property exists this function will return FWTS_OK---> *value will 
contain full arrray of u32's.
else function will return FWTS_ERROR ----> *value will contain error 
code which comes from *len


fwts_dt_property_read_u32
Returns FWTS_OK on success----*value will contain one u32
         FWTS_ERROR on error---*value will contain error code



I will add these on top of functions to understand
usage of the callers.



>> +	return FWTS_OK;
>> +}
>> +
>> +int fwts_dt_stringlist_count(fwts_framework *fw, const void *fdt,
>> +				int nodeoffset, const char *property)
>> +{
>> +    const char *list, *end;
>> +    int length, count = 0;
>> +
>> +    list = fdt_getprop(fdt, nodeoffset, property, &length);
>> +    if (!list) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "PropertyNotFound",
>> +               "Failed to get property %s rc %d", property,length);
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    end = list + length;
>> +
>> +    while (list < end) {
>> +        length = strnlen(list, end - list) + 1;
>> +
>> +        /* Abort if the last string isn't properly NUL-terminated. */
>> +        if (list + length > end)
>> +            return -1;
> 
> strnlen() will make this condition false.  We will not get a length
> past the end.
> 
> Or you are expecting we will get the strnlen's maxlen (end - list) and
> this is how you will abort the test case?
> 
> 

Here comment is misleading, we should return error if it is not properly
null terminated. Will change the error path accordingly.
yes, strnlen will return length atmost of (end - list).


>> +
>> +        list += length;
>> +        count++;
>> +    }
>> +
>> +    return count;
>> +}
>> +
>> +int validate_dt_prop_sizes(fwts_framework *fw,
>> +                    const char *prop1, int prop1_len,
>> +                    const char *prop2, int prop2_len)
>> +{
>> +    if (prop1_len == prop2_len)
>> +        return FWTS_OK;
>> +
>> +    fwts_failed(fw, LOG_LEVEL_HIGH, "SizeMismatch",
>> +        "array sizes don't match for %s len %d and %s len %d\n",
>> +        prop1, prop1_len, prop2, prop2_len);
>> +    return FWTS_ERROR;
> 
> Do you need this helper routine to just compare the given lengths?
> 

yeah, it may not required. But will reduce the duplicated code
for multiple times checking. We will keep it in our test case
power_mgmt_info.c file, as this helper may not be suitable for
device tree library

>> +}
>> +
>> +static int get_cpu_version(fwts_framework *fw, int *value)
>> +{
>> +        const char *cpus_path = "/cpus/";
>> +        int offset;
>> +        int cpu_version;int ret;
>> +
>> +        if (!fw->fdt) {
>> +                fwts_skipped(fw, "Device tree not found");
>> +                return FWTS_SKIP;
>> +        }
>> +
>> +        offset = fdt_path_offset(fw->fdt, cpus_path);
>> +        if (offset < 0) {
>> +                fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +                "/cpus node is missing");
>> +                return FWTS_ERROR;
>> +        }
>> +
>> +        offset = fdt_node_offset_by_prop_value(fw->fdt, -1, 
>> "device_type",
>> +                         "cpu", sizeof("cpu"));
>> +        if (offset < 0) {
>> +                fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +                        "cpu node is missing");
>> +                return FWTS_ERROR;
>> +        }
>> +
>> +        ret = fwts_dt_property_read_u32(fw->fdt, offset, 
>> "cpu-version",
>> +                         &cpu_version);
>> +        if (ret != FWTS_OK) {
>> +                fwts_failed(fw, LOG_LEVEL_MEDIUM, 
>> "DTPropertyReadError",
>> +                        "Failed to read property cpu-version %s",
>> +                        fdt_strerror(cpu_version));
>> +                return FWTS_ERROR;
>> +        }
>> +        *value = cpu_version;
>> +        return FWTS_OK;
>> +}
>> +
>> +int get_proc_gen(fwts_framework *fw)
>> +{
>> +    int version; int ret;
>> +    int mask = 0xFFFF0000;
>> +    int pvr;
>> +
>> +    ret = get_cpu_version(fw, &version);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "DTNoCPUVersion",
>> +            "Not able to get the CPU version");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    pvr = (mask & version) >> 16;
>> +    switch(pvr) {
>> +        /* Get CPU family and other flags based on PVR */
>> +        case PVR_TYPE_P7:
>> +        case PVR_TYPE_P7P:
>> +                proc_gen = proc_gen_p7;
>> +                break;
>> +        case PVR_TYPE_P8E:
>> +        case PVR_TYPE_P8:
>> +                proc_gen = proc_gen_p8;
>> +                break;
>> +        case PVR_TYPE_P8NVL:
>> +                proc_gen = proc_gen_p8;
>> +                break;
>> +        case PVR_TYPE_P9:
>> +                proc_gen = proc_gen_p9;
>> +                break;
>> +        default:
>> +                proc_gen = proc_gen_unknown;
>> +        }
>> +
>> +    return FWTS_OK;
>> +}
>> diff --git a/src/opal/power_mgmt_info.c b/src/opal/power_mgmt_info.c
>> new file mode 100644
>> index 0000000..93a420b
>> --- /dev/null
>> +++ b/src/opal/power_mgmt_info.c
>> @@ -0,0 +1,357 @@
>> +/*
>> + * Copyright (C) 2010-2017 Canonical
>> + * Some of this work - Copyright (C) 2016-2017 IBM
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
>> 02110-1301, USA.
>> + *
>> + */
>> +
>> +#include <fcntl.h>
>> +#include <stdlib.h>
>> +#include <sys/ioctl.h>
>> +
>> +#include "fwts.h"
>> +
>> +#ifdef HAVE_LIBFDT
>> +#include <libfdt.h>
>> +#endif
>> +
>> +#define MAX_PSTATES 256
>> +
>> +#define CPUIDLE_STATE_MAX   10
>> +
>> +enum proc_gen proc_gen;
>> +
>> +static const char *power_mgt_path = "/ibm,opal/power-mgt/";
>> +
>> +static const char *root_node = "/";
>> +
>> +/**
>> + * cmp_pstates: Compares the given two pstates and determines which
>> + *              among them is associated with a higher pstate.
>> + *
>> + * @a, at b: The pstate ids of the pstates being compared.
>> + *
>> + * Returns: -1 : If pstate associated with @a is smaller than
>> + *               the pstate associated with @b.
>> + *      0 : If pstates associated with @a and @b are equal.
>> + *      1 : If pstate associated with @a is greater than
>> + *               the pstate associated with @b.
>> + */
>> +static int (*cmp_pstates)(int a, int b);
>> +
>> +static int cmp_positive_pstates(int a, int b)
>> +{
>> +    if (a > b)
>> +        return -1;
>> +    else if (a < b)
>> +        return 1;
>> +
>> +    return 0;
>> +}
>> +
>> +static int cmp_negative_pstates(int a, int b)
>> +{
>> +    if (a < b)
>> +        return -1;
>> +    else if (a > b)
>> +        return 1;
>> +
>> +    return 0;
>> +}
> 
> This will be a challenge.  On P8, PState is signed value with +ve and
> -ve 8bit values.  But on P9 the spec removed this confusion and makes
> PState a 8bit unsigned value with 0 as max and 254 (0xfe) and min.
> 0xff is reserved.
> 


you mean on P9 above helpers will break?
I guess even if we move signed pstates to unsigned pstates
the above helpers still work.
or let me know if it breaks?


>> +static int power_mgmt_init(fwts_framework *fw)
>> +{
>> +    int ret;
>> +
>> +    if (fwts_firmware_detect() != FWTS_FIRMWARE_OPAL) {
>> +        fwts_skipped(fw,
>> +	    "The firmware type detected was non OPAL"
>> +            "so skipping the OPAL Power Management DT checks.");
>> +        return FWTS_SKIP;
>> +    }
>> +
>> +    if (!fw->fdt) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "NoDeviceTree",
>> +                    "Device tree not found");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    ret = get_proc_gen(fw);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "ProcGenFail",
>> +                "Failed to get the Processor generation");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +
>> +static int pstate_limits_test(fwts_framework *fw)
>> +{
>> +    int pstate_min, pstate_max, pstates[MAX_PSTATES], nr_pstates;
>> +    bool ok = true;
>> +    int  offset, len, ret, i;
>> +
>> +    switch (proc_gen) {
>> +    case proc_gen_p8:
>> +        cmp_pstates = cmp_negative_pstates;
>> +        break;
>> +    case proc_gen_p9:
>> +        cmp_pstates = cmp_positive_pstates;
>> +        break;
>> +    default:
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "UnknownProcessorChip",
>> +                "Unknown processor generation %d", proc_gen);
>> +        return FWTS_ERROR;
>> +    }
> 
> Lets do this way... check for monotonicity in Pstate and note min and
> max.  Depending on P8 vs P9, you can verify which direction the value
> should have increased.
> 

I think this one is implemented by verifying the monotonicity
from Pmax to Pmin with the use of above helpers.


>> +
>> +    offset = fdt_path_offset(fw->fdt, power_mgt_path);
>> +    if (offset < 0) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +            "power management node %s is missing", power_mgt_path);
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    ret = fwts_dt_property_read_u32(fw->fdt, offset, 
>> "ibm,pstate-min",
>> +                                    &pstate_min);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                    "Failed to read property ibm,pstate-min %s",
>> +                    fdt_strerror(pstate_min));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    ret = fwts_dt_property_read_u32(fw->fdt, offset, 
>> "ibm,pstate-max",
>> +                                    &pstate_max);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                    "Failed to read property ibm,pstate-max %s",
>> +                    fdt_strerror(pstate_max));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    ret = fwts_dt_property_read_u32_arr(fw->fdt, offset, 
>> "ibm,pstate-ids",
>> +                                        pstates, &len);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                    "Failed to read property ibm,pstate-ids %s",
>> +                    fdt_strerror(len));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    nr_pstates = abs(pstate_max - pstate_min + 1);
>> +
>> +    if (nr_pstates <= 1 || nr_pstates > 128)
>> +        fwts_log_warning(fw, "Pstates range %d is not valid",
>> +                         nr_pstates);
> 
> As per spec p9 can have more than 128, since the PState ID in
> unsigned.  But we want to know if it is more than 128.  It will
> certainly be an error.
> 

So i should verify on P8 it should'nt be greater than 128,
on P9 it shouldn't be greater than 255?


>> +
>> +    if (len != nr_pstates)
>> +        fwts_log_warning(fw, "Wrong number of pstates."
>> +                         "Expected %d pstates, found %d pstates",
>> +                         nr_pstates, len);
>> +
>> +    for (i = 0; i < nr_pstates; i++) {
>> +        if (cmp_pstates(pstate_max, pstates[i]) < 0) {
>> +            fwts_log_warning(fw, "Invalid Pstate id %d"
>> +                            "greater than max pstate %d",
>> +                            pstates[i], pstate_max);
>> +            ok = false;
>> +        }
>> +        if (cmp_pstates(pstates[i], pstate_min) < 0) {
>> +            fwts_log_warning(fw, "Invalid Pstate id %d"
>> +                            "lesser than min pstate %d",
>> +                            pstates[i], pstate_min);
>> +            ok = false;
>> +        }
>> +    }
>> +
>> +    /* Pstates should be in monotonic descending order */
>> +    for (i = 0; i < nr_pstates; i++) {
>> +        if ((i == 0) && (cmp_pstates(pstates[i], pstate_max) != 0)) {
>> +            fwts_log_warning(fw, "Pstates mismatch: Expected Pmin 
>> %d,"
>> +                         "Actual Pmin %d",
>> +                         pstate_min, pstates[i]);
>> +            ok = false;
>> +        }
>> +        else if((i == nr_pstates-1) &&
>> +            (cmp_pstates(pstates[i], pstate_min) != 0)) {
>> +            fwts_log_warning(fw, "Pstates mismatch: Expected Pmax 
>> %d,"
>> +                         "Actual Pmax %d",
>> +                         pstate_max, pstates[i]);
>> +            ok = false;
>> +        }
>> +        else {
>> +            int previous_pstate;
>> +            previous_pstate = pstates[i-1];
>> +            if (cmp_pstates(pstates[i], previous_pstate) > 0) {
>> +                fwts_log_warning(fw, "Non monotonicity observed,"
>> +                         "Pstate %d greater then previous Pstate %d",
>> +                         pstates[i], previous_pstate);
>> +                ok = false;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (ok)
>> +        fwts_passed(fw, "CPU Frequency pstates are validated");
>> +    else
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "CPUPstateLimitsTestFail",
>> +			"One or few CPU Pstates DT validation tests failed");
>> +    return FWTS_OK;
>> +}
>> +
>> +static int cpuidle_states_test(fwts_framework *fw)
>> +{
>> +    int offset, len, test_len, ret;
>> +    int latency_ns[CPUIDLE_STATE_MAX];
>> +    int residency_ns[CPUIDLE_STATE_MAX];
>> +    int flags[CPUIDLE_STATE_MAX];
>> +    bool has_stop_inst = false;
>> +    char *supported_states;
>> +    bool ok = true;
>> +
>> +    switch (proc_gen) {
>> +    case proc_gen_p8:
>> +        has_stop_inst = false;
>> +        supported_states = "ibm,enabled-idle-states";
>> +        break;
>> +    case proc_gen_p9:
>> +        has_stop_inst = true;
>> +        supported_states = "ibm,enabled-stop-levels";
> 
> This ibm,enabled-stop-levels is from hostboot (HDAT) to OPAL.
> OPAL->Linux should look at ibm,enabled-idle-states only as in P8.
> 
> Also the format between the 2 properties are different.
> 

ibm,enabled-idle-states is an array of strings
What is the format for ibm,enabled-stop-levels?
Here we need the length of stop levels to compare
other properties lengths in p9.


>> +        break;
>> +    default:
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "UnknownProcessorChip",
>> +                "Unknown processor generation %d", proc_gen);
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    offset = fdt_path_offset(fw->fdt, power_mgt_path);
>> +    if (offset < 0) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +                "power management node %s is missing", 
>> power_mgt_path);
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    /* Validate ibm,cpu-idle-state-flags property */
>> +    ret = fwts_dt_property_read_u32_arr(fw->fdt, offset,
>> +                "ibm,cpu-idle-state-flags", flags, &len);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                "Failed to read property ibm,cpu-idle-state-flags 
>> %s",
>> +                fdt_strerror(len));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    if (len < 0) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNoIdleStates",
>> +                    "No idle states found in DT");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    if (len > CPUIDLE_STATE_MAX-1) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTMoreIdleStates",
>> +                    "More idle states found in DT than the 
>> expected");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    /* Validate ibm,cpu-idle-state-latencies-ns property */
>> +    ret = fwts_dt_property_read_u32_arr(fw->fdt, offset,
>> +                    "ibm,cpu-idle-state-latencies-ns", latency_ns, 
>> &test_len);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                "Failed to read property 
>> ibm,cpu-idle-state-latencies-ns %s",
>> +                fdt_strerror(test_len));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    if (validate_dt_prop_sizes(fw, "ibm,cpu-idle-state-flags", len,
>> +                    "ibm,cpu-idle-state-latencies-ns", test_len) != 
>> FWTS_OK)
>> +        ok = false;
>> +
>> +    /* Validate ibm,cpu-idle-state-names property */
>> +    test_len = fwts_dt_stringlist_count(fw, fw->fdt, offset,
>> +                    "ibm,cpu-idle-state-names");
>> +    if (test_len > 0) {
>> +        if (validate_dt_prop_sizes(fw, "ibm,cpu-idle-state-flags", 
>> len,
>> +                            "ibm,cpu-idle-state-names", test_len) != 
>> FWTS_OK)
>> +            ok = false;
>> +    }
>> +
>> +    /* Validate ibm,cpu-idle-state-residency-ns property */
>> +    ret = fwts_dt_property_read_u32_arr(fw->fdt, offset,
>> +                "ibm,cpu-idle-state-residency-ns", residency_ns, 
>> &test_len);
>> +    if (ret != FWTS_OK) {
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
>> +                "Failed to read property 
>> ibm,cpu-idle-state-residency-ns %s",
>> +                fdt_strerror(test_len));
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    if (validate_dt_prop_sizes(fw, "ibm,cpu-idle-state-flags", len,
>> +                    "ibm,cpu-idle-state-residency-ns", test_len) != 
>> FWTS_OK)
>> +        ok = false;
>> +
>> +
>> +    if (has_stop_inst) {
>> +        /* In P9 ibm,enabled-stop-levels present under 
>> /ibm,opal/power-mgt/ */
>> +        test_len = fwts_dt_stringlist_count(fw, fw->fdt, offset,
>> +                            supported_states);
>> +    }
>> +    else {
>> +        /* In P8 ibm,enabled-idle-states present under root node*/
>> +        offset = fdt_path_offset(fw->fdt, root_node);
>> +        if (offset < 0) {
>> +            fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTRootNodeMissing",
>> +                    "root node %s is missing", root_node);
>> +            return FWTS_ERROR;
>> +        }
>> +        test_len = fwts_dt_stringlist_count(fw, fw->fdt, offset,
>> +                            supported_states);
>> +    }
>> +
>> +    if (test_len > 0) {
>> +        if (validate_dt_prop_sizes(fw, "ibm,cpu-idle-state-flags", 
>> len,
>> +                                supported_states, test_len) != 
>> FWTS_OK)
>> +            ok = false;
>> +    }
>> +
>> +    /* TODO: add tests for pmicr and psscr value and mask bits */
>> +
>> +    if (ok)
>> +        fwts_passed(fw, "CPU IDLE States are validated");
>> +    else
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "CPUIDLEStatesFail",
>> +                "One or few CPU IDLE DT Validation tests failed");
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +static fwts_framework_minor_test power_mgmt_tests[] = {
>> +    { pstate_limits_test, "OPAL Processor Frequency States Info" },
>> +    { cpuidle_states_test, "OPAL Processor Idle States Info" },
>> +    { NULL, NULL }
>> +};
>> +
>> +static fwts_framework_ops power_mgmt_tests_ops = {
>> +    .description = "OPAL Processor Power Management DT Validation",
>> +    .init        = power_mgmt_init,
>> +    .minor_tests = power_mgmt_tests
>> +};
>> +
>> +FWTS_REGISTER_FEATURES("power_mgmt", &power_mgmt_tests_ops, 
>> FWTS_TEST_EARLY,
>> +		FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV,
>> +		FWTS_FW_FEATURE_DEVICETREE)
> 
> Thanks for adding the test case for all the fine details in the device 
> tree.
> 
> --Vaidy



More information about the Skiboot mailing list