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

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.

Signed-off-by: Pridhiviraj Paidipeddi <ppaidipe@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

* Pridhiviraj Paidipeddi <ppaidipe@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@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?


> +	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?


> +
> +        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?

> +}
> +
> +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,@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.

> +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.

> +
> +    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.

> +
> +    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.

> +        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

Hi Vaidy

Thanks for the review.


On 2017-04-07 23:31, Vaidyanathan Srinivasan wrote:
> * Pridhiviraj Paidipeddi <ppaidipe@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@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,@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