[Skiboot] [PATCH 4/7] hdat: Rework service procesor information parsing

[Vasant Hegde]

On 01/13/2017 12:26 PM, Oliver O'Halloran wrote:
> With OpenPower machines using HDAT on P9 the SPINFO structures have been
> amended to support BMCs in addition to the FSP. This patch reworks the
> existing parsing to prepare for adding BMC support.
>
> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> ---
>   hdata/fsp.c                 | 166 ++++++++++++++++++++++++++++++--------------
>   hdata/test/p8-840-spira.dt  |   2 +-
>   hdata/test/p81-811.spira.dt |   2 +-
>   3 files changed, 117 insertions(+), 53 deletions(-)
>
> diff --git a/hdata/fsp.c b/hdata/fsp.c
> index 90e8e5b90e3c..069a2d189d9f 100644
> --- a/hdata/fsp.c
> +++ b/hdata/fsp.c
> @@ -20,36 +20,101 @@
>   #include <vpd.h>
>   #include <ccan/str/str.h>
>   #include <interrupts.h>
> +#include <inttypes.h>
>
>   #include "hdata.h"
>
> -static struct dt_node *fsp_create_node(const void *spss, int i,
> -				       struct dt_node *parent)
> +enum sp_type {
> +	SP_BAD = 0,
> +	SP_UNK,

Better SP_UNKNOWN?

> +	SP_FSP,
> +	SP_BMC,
> +};
> +
> +static const char * const sp_names[] = {
> +	"Broken", "Unknown", "FSP", "BMC",
> +};
> +
> +static enum sp_type find_service_proc_type(const struct HDIF_common_hdr *spss,
> +		int index)

There are few other places we are adding fsp specific properties. We should make 
this function as global, so that we can validate before adding those properties.


>   {
>   	const struct spss_sp_impl *sp_impl;
> -	struct dt_node *node;
> -	unsigned int mask;
> +	int hw_ver, sw_ver, flags;
> +	enum sp_type sp_type;
> +	bool functional, installed;
>
>   	/* Find an check the SP Implementation structure */
>   	sp_impl = HDIF_get_idata(spss, SPSS_IDATA_SP_IMPL, NULL);
>   	if (!CHECK_SPPTR(sp_impl)) {
> -		prerror("FSP #%d: SPSS/SP_Implementation not found !\n", i);
> -		return NULL;
> +		prerror("SP #%d: SPSS/SP_Implementation not found !\n", index);
> +		return SP_BAD;
> +	}
> +
> +	hw_ver = be16_to_cpu(sp_impl->hw_version);
> +	sw_ver = be16_to_cpu(sp_impl->sw_version);
> +	flags  = be16_to_cpu(sp_impl->func_flags);
> +
> +	switch (hw_ver) {
> +	case 0x2: /* We only support FSP2 */
> +		sp_type = SP_FSP;
> +		break;
> +	case 0x3:
> +		sp_type = SP_BMC;
> +		break;
> +	default:
> +		sp_type = SP_UNK;

better return SP_UNK so that you can avoid next if condition.

>   	}
>
> -	prlog(PR_INFO, "FSP #%d: FSP HW version %d, SW version %d,"
> -	      " chip DD%d.%d\n", i,
> -	      be16_to_cpu(sp_impl->hw_version),
> -	      be16_to_cpu(sp_impl->sw_version),
> -	      sp_impl->chip_version >> 4, sp_impl->chip_version & 0xf);
> -	mask = SPSS_SP_IMPL_FLAGS_INSTALLED | SPSS_SP_IMPL_FLAGS_FUNCTIONAL;
> -	if ((be16_to_cpu(sp_impl->func_flags) & mask) != mask) {
> -		prerror("FSP #%d: FSP not installed or not functional\n", i);
> -		return NULL;
> +	if (sp_type == SP_UNK)
> +		return SP_UNK;
> +
> +	installed  = !!(flags & SPSS_SP_IMPL_FLAGS_INSTALLED);
> +	functional = !!(flags & SPSS_SP_IMPL_FLAGS_FUNCTIONAL);
> +
> +	if (!installed || !functional) {
> +		prerror("%s #%d not usable: %sinstalled, %sfunctional\n",
> +			sp_names[sp_type], index,
> +			installed  ? "" : "not ",
> +			functional ? "" : "not ");
> +
> +		return SP_BAD;
>   	}
>
> +	prlog(PR_INFO, "%s #%d: HW version %d, SW version %d, chip DD%d.%d\n",
> +	      sp_names[sp_type], index, hw_ver, sw_ver,
> +	      sp_impl->chip_version >> 4,
> +	      sp_impl->chip_version & 0xf);
> +
> +	return sp_type;
> +}
> +
> +/*
> + * Note on DT representation of the PSI links and FSPs:
> + *
> + * We create a XSCOM node for each PSI host bridge(one per chip),
> + *
> + * This is done in spira.c
> + *
> + * We do not create the /psi MMIO variant at this stage, it will
> + * be added by the psi driver in skiboot.
> + *
> + * We do not put the FSP(s) as children of these. Instead, we create
> + * a top-level /fsps node with the FSPs as children.
> + *
> + * Each FSP then has a "links" property which is an array of chip IDs
> + */
> +
> +static struct dt_node *fsp_create_node(const void *spss, int i,
> +				       struct dt_node *parent)
> +{
> +	const struct spss_sp_impl *sp_impl;
> +	struct dt_node *node;
> +
> +	sp_impl = HDIF_get_idata(spss, SPSS_IDATA_SP_IMPL, NULL);
> +
>   	node = dt_new_addr(parent, "fsp", i);
>   	assert(node);
> +
>   	dt_add_property_cells(node, "reg", i);
>
>   	if (be16_to_cpu(sp_impl->hw_version) == 1) {
> @@ -97,8 +162,8 @@ static uint32_t fsp_create_link(const struct spss_iopath *iopath, int index,
>   	default:
>   		ststr = "Unknown";
>   	}
> -	prlog(PR_DEBUG, "FSP #%d: IO PATH %d is %s PSI Link, GXHB at %llx\n",
> -	      fsp_index, index, ststr, (long long)be64_to_cpu(iopath->psi.gxhb_base));
> +	prlog(PR_DEBUG, "FSP #%d: IO PATH %d is %s PSI Link, GXHB at %" PRIx64 "\n",
> +	      fsp_index, index, ststr, be64_to_cpu(iopath->psi.gxhb_base));
>
>   	chip_id = pcid_to_chip_id(be32_to_cpu(iopath->psi.proc_chip_id));
>   	node = dt_find_compatible_node_on_chip(dt_root, NULL, "ibm,psihb-x",
> @@ -160,43 +225,42 @@ static void fsp_create_links(const void *spss, int index,
>
>   void fsp_parse(void)
>   {
> -	const void *base_spss, *spss;
> -	struct dt_node *fsp_root, *fsp_node;
> -	int i;
> -
> -	/*
> -	 * Note on DT representation of the PSI links and FSPs:
> -	 *
> -	 * We create a XSCOM node for each PSI host bridge(one per chip),
> -	 *
> -	 * This is done in spira.c
> -	 *
> -	 * We do not create the /psi MMIO variant at this stage, it will
> -	 * be added by the psi driver in skiboot.
> -	 *
> -	 * We do not put the FSP(s) as children of these. Instead, we create
> -	 * a top-level /fsps node with the FSPs as children.
> -	 *
> -	 * Each FSP then has a "links" property which is an array of chip IDs
> -	 */
> -
> -	/* Find SPSS in SPIRA */
> -	base_spss = get_hdif(&spira.ntuples.sp_subsys, SPSS_HDIF_SIG);
> -	if (!base_spss) {
> -		prlog(PR_WARNING, "FSP: No SPSS in SPIRA !\n");
> +	struct dt_node *fsp_root = NULL, *fsp_node;
> +	const void *sp;
> +	int index;
> +
> +	/* Find SPSS tuple in SPIRA */
> +	sp = get_hdif(&spira.ntuples.sp_subsys, SPSS_HDIF_SIG);
> +	if (!sp) {
> +		prlog(PR_WARNING, "HDAT: No FSP/BMC found!\n");
>   		return;
>   	}
>
> -	fsp_root = dt_new(dt_root, "fsps");
> -	assert(fsp_root);
> -	dt_add_property_cells(fsp_root, "#address-cells", 1);
> -	dt_add_property_cells(fsp_root, "#size-cells", 0);
> +	for_each_ntuple_idx(&spira.ntuples.sp_subsys, sp, index, SPSS_HDIF_SIG) {
> +		switch (find_service_proc_type(sp, index)) {
> +		case SP_FSP:
> +			if (!fsp_root) {
> +				fsp_root = dt_new(dt_root, "fsps");
> +				assert(fsp_root);

Better to use newly added dt_check_new() so that we can avoid redundant checks.


-Vasant