Re: [RFC PATCH 6/8] coresight: etm4x: syscfg: Add resource management to etm4x.

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Good day,

Comments for this patch will come over multiple days...

On Wed, May 12, 2021 at 10:17:50PM +0100, Mike Leach wrote:
> Adds resource management to configuration and feature handling for ETM4
> using the system configuration resource management API.
> 
> Allows specification of ETM4 resources when creating configurations
> and features.
> 
> Adds in checking and validation of resources used by features to
> prevent over allocation when multiple features used in a configuration.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  .../hwtracing/coresight/coresight-etm4x-cfg.c | 533 ++++++++++++++++++
>  .../hwtracing/coresight/coresight-etm4x-cfg.h | 196 ++++++-
>  include/linux/coresight.h                     |   2 +
>  3 files changed, 724 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> index d2ea903231b2..ba6d20b58a9a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> @@ -28,6 +28,11 @@
>  		} \
>  	}
>  
> +/* check for a match to ts rate */
> +static bool etm4_cfg_feat_is_ts_rate(const struct cscfg_feature_desc *feat_desc);
> +
> +#define TS_RATE_REG_VAL_IDX 0
> +
>  /**
>   * etm4_cfg_map_reg_offset - validate and map the register offset into a
>   *			     location in the driver config struct.
> @@ -128,6 +133,66 @@ static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata,
>  	return err;
>  }
>  
> +static void etm4_cfg_dump_res_mask(const char *name, struct etm4_cfg_resources *res)
> +{
> +	pr_debug("Mask %s\n", name);
> +	pr_debug("selectors %08x; addr_cmp %04x\n", res->selectors, res->addr_cmp);
> +	pr_debug("cid_cmp %02x; vmid_cmp %02x; counters %02x\n",  res->cid_cmp,
> +		 res->vmid_cmp, res->counters);
> +	pr_debug("misc bits %08x\n", res->misc);

Even at a pr_debug() log level this is probably too much output.  I also think
this is a purely debugging mechanism and only used when developing new features.
I would enclose the core of the function with an #if DEBUG and add #define DEBUG
0 at the top of the file.

> +}
> +
> +/*
> + * generate an address offset from a resource type and index
> + * Bit selected resources will return a ETM4_RES_OFFSET_SKIP value
> + * as these require special handling on enable / disable.
> + */
> +static u32 etm4_cfg_get_res_offset(u16 res_type, u32 res_idx)
> +{
> +	u32 offset = ETM4_RES_OFFSET_ERR;
> +
> +	switch (res_type & ETM4_CFG_RES_MASK) {
> +	case ETM4_CFG_RES_CTR:
> +		if (res_type & ETM4_CTR_VAL)
> +			offset = TRCCNTVRn(res_idx);
> +		else if (res_type & ETM4_CTR_RLD)
> +			offset = TRCCNTRLDVRn(res_idx);
> +		else if (res_type & ETM4_CTR_CTRL)
> +			offset = TRCCNTCTLRn(res_idx);
> +		break;
> +	case ETM4_CFG_RES_CMP:
> +		if (res_type & ETM4_CMP_VAL)
> +			offset = TRCACVRn(res_idx);
> +		else if (res_type & ETM4_CMP_CTL)
> +			offset = TRCACATRn(res_idx);
> +		break;
> +	case ETM4_CFG_RES_SEL:
> +		offset = TRCRSCTLRn(res_idx);
> +		break;
> +

The above case statements don't have a space and the ones below do.  The same
applies to function etm4_cfg_update_res_from_des().  I don't really mind which
one is used (though I'd prefer no space) but pick a scheme and stick with it.

> +	case ETM4_CFG_RES_SEQ:
> +		if (res_type & ETM4_SEQ_STATE_R)
> +			offset = TRCSEQEVRn(res_idx);
> +		else if (res_type & ETM4_SEQ_RESET_R)
> +			offset = TRCSEQRSTEVR;
> +		break;
> +	case ETM4_CFG_RES_CID_CMP:
> +		offset = TRCCIDCVRn(res_idx);
> +		break;
> +
> +	case ETM4_CFG_RES_VID_CMP:
> +		offset = TRCVMIDCVRn(res_idx);
> +		break;
> +
> +		/* these two have dedicated enable functions, no address needed */
> +	case ETM4_CFG_RES_BITCTRL:
> +	case ETM4_CFG_RES_TS:
> +		offset = ETM4_RES_OFFSET_SKIP;
> +		break;
> +	}
> +	return offset;
> +}
> +
>  /**
>   * etm4_cfg_load_feature - load a feature into a device instance.
>   *
> @@ -163,11 +228,349 @@ static int etm4_cfg_load_feature(struct coresight_device *csdev,
>  	/* process the register descriptions */
>  	for (i = 0; i < feat_csdev->nr_regs && !err; i++) {
>  		offset = feat_desc->regs_desc[i].offset;
> +
> +		/* resource needs conversion to a register access value */
> +		if (feat_desc->regs_desc[i].type & CS_CFG_REG_TYPE_RESOURCE) {
> +			offset = etm4_cfg_get_res_offset(feat_desc->regs_desc[i].hw_info,
> +							 offset);
> +			if (offset == ETM4_RES_OFFSET_ERR) {
> +				err = -ENODEV;
> +				break;
> +			} else if (offset == ETM4_RES_OFFSET_SKIP)
> +				continue;
> +		}
>  		err = etm4_cfg_map_reg_offset(drvdata, &feat_csdev->regs_csdev[i], offset);
>  	}
>  	return err;
>  }
>  
> +/*
> + * ts rate - set a counter to emit timestamp requests at a set interval.
> + *	if we have sufficient resources then we use a counter and resource
> + *	selector to achieve this.
> + *
> + *	However, if not then do the best possible - which prevents the perf
> + *	event timestamp request from failing if any configuration selection
> + *	is using resources. e.g. when profiling, timestamps do not really matter.
> + */
> +void etm4_cfg_set_ts_rate(struct coresight_device *csdev, u32 ts_rate_val)
> +{
> +	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct etmv4_config *drvcfg = &drvdata->config;
> +	struct cscfg_res_impl_used *res_impl_used;
> +	int counter_idx, res_sel_idx;
> +	u32 tsctlr_val = 0;
> +
> +	res_impl_used = (struct cscfg_res_impl_used *)csdev->cscfg_res_mask;
> +
> +	/* look for resources */
> +	counter_idx = etm4_res_find_counter(res_impl_used);
> +	res_sel_idx = etm4_res_find_selector(res_impl_used);
> +	if (counter_idx >= 0 && res_sel_idx >= 0) {
> +		/* counter and selector - can set up ts rate normally */
> +		/*
> +		 * counter @ 1 and reload @ rate supplied -
> +		 * immediate timestamp then every rate
> +		 */
> +		drvcfg->cntr_val[counter_idx] = 1;
> +		drvcfg->cntrldvr[counter_idx] = ts_rate_val;
> +		/*
> +		 * counter ctrl - bit 16: 1 for reload self,
> +		 * bit 7: 0 single event,
> +		 * bit 6:0 res sel 1 - true
> +		 */
> +		drvcfg->cntr_ctrl[counter_idx] = 0x1 << 16 | 0x1;
> +
> +		/*
> +		 * set up resource selector for the counter.
> +		 * bits 19:16 - group 0b0010 counter
> +		 * bits 15:0 - bit select for counter idx
> +		 */
> +		drvcfg->res_ctrl[res_sel_idx] = (0x2 << 16) | (0x1 << counter_idx);
> +
> +		/* single selector bit 7 == 0, bit 6:0 - selector index */
> +		tsctlr_val = res_sel_idx;
> +
> +	} else if (ts_rate_val == 1) {
> +		/*
> +		 * perf always tries to use a min value -
> +		 * emulate by setting the ts event to true
> +		 */
> +		/* single selector bit 7 == 0, bit 6:0 - selector 1 - always true */
> +		tsctlr_val = 0x1;
> +	}
> +
> +	/* set the configr reg to enable TS, and the ts control reg */
> +	drvcfg->ts_ctrl = tsctlr_val;
> +	drvcfg->cfg |= BIT(11);
> +}
> +
> +/*
> + * on enable a feature - called after generic routine has programmed other registers.
> + * handle bit selects and custom elements
> + */
> +static int etm4_cfg_on_enable_feat(struct cscfg_feature_csdev *feat_csdev)
> +{
> +	int err = 0;
> +	struct etm4_cfg_resources *res_feat;
> +	struct device *dev = feat_csdev->csdev->dev.parent;
> +	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> +	struct etmv4_config *drvcfg = &drvdata->config;
> +	u32 ts_rate_val;
> +
> +	/*
> +	 * look for the bit selected resources in this feature and set driver
> +	 * values to be programmed when enabling hardware.
> +	 */
> +	res_feat = (struct etm4_cfg_resources *)feat_csdev->res_used;
> +
> +	/* if none of the bit selected resources in use, exit early */
> +	if (!res_feat->misc)
> +		return 0;
> +
> +	/* otherwise check each and set as required */
> +	if (res_feat->ctxt_id)
> +		drvcfg->cfg |= BIT(6);
> +
> +	if (res_feat->vm_id)
> +		drvcfg->cfg |= BIT(7);
> +
> +	/* return stack is bit 12 in config register */
> +	if (res_feat->return_stack)
> +		drvcfg->cfg |= BIT(12);
> +
> +	/* branch broadcast - feature using this must program the bbctlr */
> +	if (res_feat->branch_broadcast)
> +		drvcfg->cfg |= BIT(3);
> +
> +	/* cycle count */
> +	if (res_feat->cycle_cnt) {
> +		drvcfg->cfg |= BIT(4);
> +		/* TRM: Must program this for cycacc to work - ensure mun permitted */
> +		if (drvcfg->ccctlr < drvdata->ccitmin)
> +			drvcfg->ccctlr = drvdata->ccitmin;
> +	}
> +
> +	/*
> +	 * timestamps - if not ts-rate just set to on, otherwise
> +	 * set using reload counter according to requested rate
> +	 */
> +	if (res_feat->timestamp) {
> +		/* the current feature is the ts-rate feature */
> +		if (res_feat->ts_rate) {
> +			ts_rate_val = feat_csdev->regs_csdev[TS_RATE_REG_VAL_IDX].reg_desc.val32;
> +			etm4_cfg_set_ts_rate(feat_csdev->csdev, ts_rate_val);
> +		} else
> +			drvcfg->cfg |= BIT(11);
> +	}
> +	return err;
> +}
> +
> +/* set the overall available resource masks for the device */
> +static int etm4_cfg_set_res_mask(struct coresight_device *csdev)
> +{
> +	struct device *dev = csdev->dev.parent;
> +	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> +	struct etm4_cfg_resources *res;
> +	struct cscfg_res_impl_used *res_impl_used;
> +
> +	res_impl_used = devm_kzalloc(dev, sizeof(*res_impl_used), GFP_KERNEL);
> +	if (!res_impl_used)
> +		return -ENOMEM;
> +	res = &res_impl_used->impl;
> +
> +	/* selectors */
> +	if (drvdata->nr_resource)
> +		res->selectors = GENMASK((drvdata->nr_resource * 2) - 1, 0);
> +
> +	/* comparators */
> +	if (drvdata->nr_addr_cmp)
> +		res->addr_cmp = GENMASK(drvdata->nr_addr_cmp - 1, 0);
> +
> +	if (drvdata->numvmidc)
> +		res->vmid_cmp = GENMASK(drvdata->numvmidc - 1, 0);
> +
> +	if (drvdata->numcidc)
> +		res->cid_cmp = GENMASK(drvdata->numcidc - 1, 0);
> +
> +	/* misc resources */
> +	if (drvdata->nr_cntr)
> +		res->counters = GENMASK(drvdata->nr_cntr - 1, 0);
> +
> +	if (drvdata->trccci)
> +		res->cycle_cnt = 1;
> +
> +	if (drvdata->trcbb)
> +		res->branch_broadcast = 1;
> +
> +	if (drvdata->ctxid_size)
> +		res->ctxt_id = 1;
> +
> +	if (drvdata->vmid_size)
> +		res->vm_id = 1;
> +
> +	if (drvdata->nrseqstate)
> +		res->sequencer = 1;
> +
> +	if (drvdata->retstack)
> +		res->return_stack = 1;
> +
> +	if (drvdata->ts_size) {
> +		res->timestamp = 1;
> +		if (drvdata->nr_cntr && drvdata->nr_resource)
> +			res->ts_rate = 1;
> +	}
> +	etm4_cfg_dump_res_mask("device impl resources", &res_impl_used->impl);
> +	csdev->cscfg_res_mask = res_impl_used;
> +	return 0;
> +}
> +
> +/*
> + *  reads a descriptor and updates the resource mask structure
> + *  checks resource indexes are valid.
> + */
> +static int etm4_cfg_update_res_from_desc(const struct cscfg_feature_desc *feat_desc,
> +					 struct etm4_cfg_resources *res)
> +{
> +	struct cscfg_regval_desc *regs_desc = &feat_desc->regs_desc[0];
> +	u32 res_idx, hw_info;
> +	int i;
> +
> +	for (i = 0; i < feat_desc->nr_regs; i++) {
> +		if (regs_desc[i].type & CS_CFG_REG_TYPE_RESOURCE) {
> +			res_idx = regs_desc[i].offset;
> +			hw_info = regs_desc[i].hw_info;
> +			switch (hw_info & ETM4_CFG_RES_MASK) {
> +			case ETM4_CFG_RES_CTR:
> +				if (res_idx >= ETMv4_MAX_CNTR)
> +					goto invalid_resource_err;
> +				res->counters |= BIT(res_idx);
> +				break;
> +
> +			case ETM4_CFG_RES_CMP:
> +				if (res_idx >= ETM_MAX_SINGLE_ADDR_CMP)
> +					goto invalid_resource_err;
> +				res->addr_cmp |= BIT(res_idx);
> +				break;
> +
> +			case ETM4_CFG_RES_SEL:
> +				if (res_idx >= ETM_MAX_RES_SEL)
> +					goto invalid_resource_err;
> +				res->selectors |= BIT(res_idx);
> +				break;
> +
> +			case ETM4_CFG_RES_SEQ:
> +				res->sequencer = 1;
> +				break;
> +
> +			case ETM4_CFG_RES_TS:
> +				res->timestamp = 1;
> +				if (etm4_cfg_feat_is_ts_rate(feat_desc))
> +					res->ts_rate = 1;
> +				break;
> +
> +			case ETM4_CFG_RES_BITCTRL:
> +				if (hw_info & ETM4_BITCTRL_BRANCH_BROADCAST)
> +					res->branch_broadcast = 1;
> +				if (hw_info & ETM4_BITCTRL_CYCLE_COUNT)
> +					res->cycle_cnt = 1;
> +				if (hw_info & ETM4_BITCTRL_CTXTID)
> +					res->ctxt_id = 1;
> +				if (hw_info & ETM4_BITCTRL_VMID)
> +					res->vm_id = 1;
> +				if (hw_info & ETM4_BITCTRL_RETSTACK)
> +					res->return_stack = 1;
> +				break;
> +
> +			case ETM4_CFG_RES_CID_CMP:
> +				if (res_idx >= ETMv4_MAX_CTXID_CMP)
> +					goto invalid_resource_err;
> +				res->cid_cmp |= BIT(res_idx);
> +				break;
> +
> +			case ETM4_CFG_RES_VID_CMP:
> +				if (res_idx >= ETM_MAX_VMID_CMP)
> +					goto invalid_resource_err;
> +				res->vmid_cmp |= BIT(res_idx);
> +				break;
> +			}
> +		}
> +	}
> +	return 0;
> +
> +invalid_resource_err:
> +	pr_err("Error: Invalid resource values in feature %s\n", feat_desc->name);
> +	return -EINVAL;
> +}
> +/*
> + * Check that the device contains the minimum resources required to support the
> + * described @feat_desc. Return -ENODEV if missing required resources.
> + */
> +static int etm4_cfg_check_feat_res(struct coresight_device *csdev,
> +				   struct cscfg_feature_desc *feat_desc)
> +{
> +	struct etm4_cfg_resources req_res;
> +	struct cscfg_res_impl_used *dev_res;
> +	int err;
> +
> +	/* create a resource mask from descriptor and validate */
> +	memset(&req_res, 0, sizeof(req_res));
> +	err = etm4_cfg_update_res_from_desc(feat_desc, &req_res);
> +	etm4_cfg_dump_res_mask("check_feat_res", &req_res);
> +	if (!err) {
> +		dev_res = (struct cscfg_res_impl_used *)csdev->cscfg_res_mask;
> +		if (!etm4_cfg_check_impl(&dev_res->impl, &req_res))
> +			return -ENODEV;
> +	}
> +	return err;
> +}
> +
> +/*
> + * Allocate resource requirements for the feature before
> + * it is programmed into the system. Ensures that two or more features in a
> + * configuration do not try to use the same resources on the device.
> + *
> + * At this point we use the absolute programmed resources - we do not attempt
> + * to find alternate available resources. (e.g. if 2 features use selector 3,
> + * fail the 2nd feature - do not look for an alternative free selector).
> + */
> +static int etm4_cfg_alloc_feat_res(struct cscfg_feature_csdev *feat_csdev)
> +{
> +	struct coresight_device *csdev = feat_csdev->csdev;
> +	struct device *dev = csdev->dev.parent;
> +	struct etm4_cfg_resources *res_feat, *res_inuse;
> +	int err = 0;
> +
> +	/* one off initialisation of resources required for this feature */
> +	if (!feat_csdev->res_used) {
> +		res_feat = devm_kzalloc(dev, sizeof(*res_feat), GFP_KERNEL);
> +		if (!res_feat)
> +			return -ENOMEM;
> +		err = etm4_cfg_update_res_from_desc(feat_csdev->feat_desc, res_feat);
> +		if (err)
> +			return err;
> +		feat_csdev->res_used = res_feat;
> +	} else
> +		res_feat = (struct etm4_cfg_resources *)feat_csdev->res_used;
> +
> +	/* check that the device resources reqiured are not in use */
> +	res_inuse = &((struct cscfg_res_impl_used *)csdev->cscfg_res_mask)->used;
> +	if (!etm4_cfg_check_set_inuse(res_inuse, res_feat))
> +		err = -ENOSPC;
> +
> +	return err;
> +}
> +
> +static void etm4_cfg_clear_feat_res(struct cscfg_feature_csdev *feat_csdev)
> +{
> +	struct coresight_device *csdev = feat_csdev->csdev;
> +	struct etm4_cfg_resources *res_feat, *res_inuse;
> +
> +	res_feat = (struct etm4_cfg_resources *)feat_csdev->res_used;
> +	res_inuse = &((struct cscfg_res_impl_used *)csdev->cscfg_res_mask)->used;
> +	etm4_cfg_clear_inuse(res_inuse, res_feat);
> +}
> +
>  /* match information when loading configurations */
>  #define CS_CFG_ETM4_MATCH_FLAGS	(CS_CFG_MATCH_CLASS_SRC_ALL | \
>  				 CS_CFG_MATCH_CLASS_SRC_ETM4)
> @@ -175,8 +578,138 @@ static int etm4_cfg_load_feature(struct coresight_device *csdev,
>  int etm4_cscfg_register(struct coresight_device *csdev)
>  {
>  	struct cscfg_csdev_feat_ops ops;
> +	int err = 0;
> +
> +	err = etm4_cfg_set_res_mask(csdev);
> +	if (err)
> +		return err;
>  
>  	ops.load_feat = &etm4_cfg_load_feature;
> +	ops.check_feat_res = &etm4_cfg_check_feat_res;
> +	ops.alloc_feat_res = &etm4_cfg_alloc_feat_res;
> +	ops.clear_feat_res = &etm4_cfg_clear_feat_res;
> +	ops.set_on_enable = &etm4_cfg_on_enable_feat;
> +	ops.clear_on_disable = 0;

s/0/NULL

>  
>  	return cscfg_register_csdev(csdev, CS_CFG_ETM4_MATCH_FLAGS, &ops);
>  }
> +
> +/*
> + * find first available bit in implemented mask @impl, that is not set in @used mask.
> + * set bit in @used and return. Return -ENOSPC if no available bits.
> + */
> +int etm4_cfg_find_unused_idx(unsigned long *impl, unsigned long *used, int size)
> +{
> +	unsigned long end_idx, unused_idx;
> +
> +	end_idx = find_first_zero_bit(impl, size);
> +	unused_idx = find_first_zero_bit(used, size);
> +	if (unused_idx < end_idx) {
> +		*used |= BIT(unused_idx);
> +		return (int)unused_idx;
> +	}
> +	return -ENOSPC;
> +}
> +
> +/*
> + * find first available pair of bits in implemented mask @impl, that are not set in
> + * @used mask. First bit of pair will always be an even index.
> + * Set bits in @used and return. Return -ENOSPC if no available bits.
> + */
> +int etm4_cfg_find_unused_idx_pair(unsigned long *impl, unsigned long *used, int size)
> +{
> +	unsigned long end_idx, first_unused_idx, next_unused_idx;
> +
> +	end_idx = find_first_zero_bit(impl, size);
> +	first_unused_idx = find_first_zero_bit(used, size);
> +
> +	/*
> +	 * even indexes are the 1st in a pair, look through the comparators
> +	 * till a pair found or we are at the end of the list.
> +	 */
> +	while (first_unused_idx < end_idx) {
> +		/* first is an even number, if the next is free we have a pair */
> +		if (!(first_unused_idx % 2)) {
> +			next_unused_idx = find_next_zero_bit(used, size, first_unused_idx);
> +			if (next_unused_idx == (first_unused_idx + 1)) {
> +				*used |= BIT(first_unused_idx);
> +				*used |= BIT(next_unused_idx);
> +				return (int)first_unused_idx;
> +			}
> +			first_unused_idx = next_unused_idx;
> +		} else
> +			first_unused_idx = find_next_zero_bit(used, size, first_unused_idx);
> +	}
> +	return -ENOSPC;
> +}
> +
> +
> +/* built in timestamp rate for etm4x */
> +static struct cscfg_parameter_desc ts_rate_param[] = {
> +	{
> +		.name = "ts_rate_cycles",
> +		.value = 100.
> +	},
> +};
> +
> +static struct cscfg_regval_desc ts_rate_regs[] = {
> +	{
> +		.type = CS_CFG_REG_TYPE_RESOURCE | CS_CFG_REG_TYPE_VAL_PARAM,
> +		.offset = 0,
> +		.hw_info = ETM4_CFG_RES_TS,
> +		.param_idx = 0,
> +	},
> +};
> +
> +static struct cscfg_feature_desc ts_rate_etm4x = {
> +	.name = "timestamp-rate",
> +	.description = "Enable timestamps and set rate they appear in the trace.\n"
> +	"Rate value is number of cycles between timestamp requests. Min value 1.\n",
> +	.match_flags = CS_CFG_MATCH_CLASS_SRC_ETM4,
> +	.nr_params = ARRAY_SIZE(ts_rate_param),
> +	.params_desc = ts_rate_param,
> +	.nr_regs = ARRAY_SIZE(ts_rate_regs),
> +	.regs_desc = ts_rate_regs,
> +};
> +
> +static struct cscfg_feature_desc *etm4x_feats[] = {
> +	&ts_rate_etm4x,
> +	NULL,
> +};
> +
> +static struct cscfg_config_desc *etm4x_cfgs[] = {
> +	NULL,
> +};
> +
> +static struct cscfg_load_owner_info etm4x_mod_owner = {
> +	.type = CSCFG_OWNER_MODULE,
> +	.owner_handle = THIS_MODULE,
> +};
> +
> +/*
> + * check if incoming feature is ts-rate
> + */
> +static bool etm4_cfg_feat_is_ts_rate(const struct cscfg_feature_desc *feat_desc)
> +{
> +	if (!strcmp(feat_desc->name, ts_rate_etm4x.name))
> +		return true;
> +	return false;
> +}
> +
> +/* load the etm4 builtin ts_rate feature into the system */
> +int etm4_cscfg_load_builtin_cfg(void)
> +{
> +	int err;
> +
> +	err = cscfg_load_config_sets(etm4x_cfgs, etm4x_feats, &etm4x_mod_owner);
> +
> +	/* if currently loaded matching devs ts_rate, still allow to load */
> +	if (err == -ENODEV)
> +		err = 0;
> +	return err;
> +}
> +
> +void etm4_cscfg_unload_builtin_cfg(void)
> +{
> +	cscfg_unload_config_sets(&etm4x_mod_owner);
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.h b/drivers/hwtracing/coresight/coresight-etm4x-cfg.h
> index 32dab34c1dac..dd69a8ef522d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.h
> @@ -13,18 +13,200 @@
>  
>  /* resource IDs */
>  
> +/*
> + * 12 bit resource ID:
> + * 3:0 = resource type in use.
> + * 11:4 = additional resource specific information.
> + */
>  #define ETM4_CFG_RES_CTR	0x001
>  #define ETM4_CFG_RES_CMP	0x002
> -#define ETM4_CFG_RES_CMP_PAIR0	0x003
> -#define ETM4_CFG_RES_CMP_PAIR1	0x004
> -#define ETM4_CFG_RES_SEL	0x005
> -#define ETM4_CFG_RES_SEL_PAIR0	0x006
> -#define ETM4_CFG_RES_SEL_PAIR1	0x007
> -#define ETM4_CFG_RES_SEQ	0x008
> -#define ETM4_CFG_RES_TS		0x009
> +#define ETM4_CFG_RES_SEL	0x003
> +#define ETM4_CFG_RES_SEQ	0x004
> +#define ETM4_CFG_RES_TS		0x005
> +#define ETM4_CFG_RES_BITCTRL	0x006
> +#define ETM4_CFG_RES_CID_CMP	0x007
> +#define ETM4_CFG_RES_VID_CMP	0x008
>  #define ETM4_CFG_RES_MASK	0x00F
>  
> +/* additional bits to supplement _CFG_RES_CTR */
> +#define ETM4_CTR_VAL	0x010
> +#define ETM4_CTR_RLD	0x020
> +#define ETM4_CTR_CTRL	0x040
> +
> +/* additional bits for address comparators _CFG_RES_CMP */
> +#define ETM4_CMP_PAIR0	0x010
> +#define ETM4_CMP_PAIR1	0x020
> +#define ETM4_CMP_VAL	0x040
> +#define ETM4_CMP_CTL	0x080
> +
> +/* additional bits for resource selectors _CFG_RES_SEL */
> +#define ETM4_SEL_PAIR0	0x010
> +#define ETM4_SEL_PAIR1	0x020
> +
> +/* addtional bits for sequencer _CFG_RES_SEQ */
> +#define ETM4_SEQ_STATE_R	0x010
> +#define ETM4_SEQ_RESET_R        0x020
> +
> +/* additional bits to supplement _CFG_RES_BITCTRL */
> +#define ETM4_BITCTRL_BRANCH_BROADCAST	0x010
> +#define ETM4_BITCTRL_CYCLE_COUNT	0x020
> +#define ETM4_BITCTRL_CTXTID		0x040
> +#define ETM4_BITCTRL_VMID		0x080
> +#define ETM4_BITCTRL_RETSTACK		0x100
> +
> +/* error value when calculating resource register offset (max offset = 0xFFC) */
> +#define ETM4_RES_OFFSET_ERR	0xFFF
> +
> +/* skip value if a bit control that is resolved later */
> +#define ETM4_RES_OFFSET_SKIP	0xFFE
> +
> +/**
> + * Masks to indicate resource usage.
> + * @selectors: The resource selector regs - max 32 off
> + * @comparators: Comparators - address (16 max), context ID (8 max), VMID (8 max).
> + * @misc:-  bitselected features, sequencer etc.

There is a discrepancy between the above and the structure itself.

> + */
> +struct etm4_cfg_resources {

I would name this etm4_cfg_resources_mask.

> +	u32 selectors;
> +	u16 addr_cmp;
> +	u8 cid_cmp;
> +	u8 vmid_cmp;
> +	u8 counters;
> +	union {
> +		u32 misc;
> +		struct {
> +			u32 cycle_cnt:1;
> +			u32 branch_broadcast:1;
> +			u32 ctxt_id:1;
> +			u32 vm_id:1;
> +			u32 sequencer:1;
> +			u32 return_stack:1;
> +			u32 timestamp:1;
> +			u32 ts_rate:1;
> +		};
> +	};
> +};
> +
> +/* structure to hold implemented & used resources for the coresight device */
> +struct cscfg_res_impl_used {

And this etm4_cfg_resources.

Also, I find it quite confusing that some structure and functions start with
etm4_cfg_ while other with etm4_cscfg_.  Having a uniform naming convention
would be greatly appreciated.

More comments tomorrow...

> +	struct etm4_cfg_resources impl;
> +	struct etm4_cfg_resources used;
> +};
> +
> +/* resource mask tests */
> +/* check implmented - ensure that all bits in @req exist in @impl */
> +static inline bool etm4_cfg_check_impl(struct etm4_cfg_resources *impl,
> +				       struct etm4_cfg_resources *req)
> +{
> +	/* invert impl then and req - anything set is outside impl mask */
> +	if ((~impl->selectors & req->selectors) ||
> +	    (~impl->addr_cmp & req->addr_cmp) ||
> +	    (~impl->cid_cmp & req->cid_cmp) ||
> +	    (~impl->vmid_cmp & req->vmid_cmp) ||
> +	    (~impl->counters & req->counters) ||
> +	    (~impl->misc & req->misc))
> +		return false;
> +	return true;
> +}
> +
> +/* check @req not @inuse, & set @inuse if free (assumes @req passed the impl check) */
> +static inline bool etm4_cfg_check_set_inuse(struct etm4_cfg_resources *inuse,
> +					    struct etm4_cfg_resources *req)
> +{
> +	/* first check for hits between inuse and requested bits */
> +	if ((inuse->selectors & req->selectors) ||
> +	    (inuse->addr_cmp & req->addr_cmp) ||
> +	    (inuse->cid_cmp & req->cid_cmp) ||
> +	    (inuse->vmid_cmp & req->vmid_cmp) ||
> +	    (inuse->counters & req->counters) ||
> +	    (inuse->misc & req->misc))
> +		return false;
> +
> +	/* set all requested bits as inuse */
> +	inuse->selectors |= req->selectors;
> +	inuse->addr_cmp |= req->addr_cmp;
> +	inuse->cid_cmp |= req->cid_cmp;
> +	inuse->vmid_cmp |= req->vmid_cmp;
> +	inuse->counters |= req->counters;
> +	inuse->misc |= req->misc;
> +	return true;
> +}
> +
> +static inline void etm4_cfg_clear_inuse(struct etm4_cfg_resources *inuse,
> +					struct etm4_cfg_resources *req)
> +{
> +	/* clear requested bits from inuse */
> +	inuse->selectors &= ~req->selectors;
> +	inuse->addr_cmp &= ~req->addr_cmp;
> +	inuse->cid_cmp &= ~req->cid_cmp;
> +	inuse->vmid_cmp &= ~req->vmid_cmp;
> +	inuse->counters &= ~req->counters;
> +	inuse->misc &= ~req->misc;
> +}
> +
>  /* ETMv4 specific config functions */
>  int etm4_cscfg_register(struct coresight_device *csdev);
> +int etm4_cfg_find_unused_idx(unsigned long *impl, unsigned long *used, int size);
> +int etm4_cfg_find_unused_idx_pair(unsigned long *impl, unsigned long *used, int size);
> +void etm4_cfg_set_ts_rate(struct coresight_device *csdev, u32 ts_rate_val);
> +
> +/* register etm4x builtins with cscfg on module load */
> +int etm4_cscfg_load_builtin_cfg(void);
> +void etm4_cscfg_unload_builtin_cfg(void);
> +
> +/*
> + * Set of functions to find an available resource from @res->impl, not already marked as used
> + * in @res->used.
> + * return index and mark as used in @res->used. return -ENOSPC if nothing available.
> + */
> +
> +static inline int etm4_res_find_selector(struct cscfg_res_impl_used *res)
> +{
> +	unsigned long *impl, *used;
> +
> +	if (!res->impl.selectors)
> +		return -ENOSPC;
> +
> +	impl = (unsigned long *)&res->impl.selectors;
> +	used = (unsigned long *)&res->used.selectors;
> +	return etm4_cfg_find_unused_idx(impl, used, ETM_MAX_RES_SEL);
> +}
> +
> +static inline int etm4_res_find_counter(struct cscfg_res_impl_used *res)
> +{
> +	unsigned long *impl, *used;
> +
> +	if (!res->impl.counters)
> +		return -ENOSPC;
> +
> +	impl = (unsigned long *)&res->impl.counters;
> +	used = (unsigned long *)&res->used.counters;
> +	return etm4_cfg_find_unused_idx(impl, used, ETMv4_MAX_CNTR);
> +}
> +
> +static inline int etm4_res_find_addr_comparator(struct cscfg_res_impl_used *res)
> +{
> +	unsigned long *impl, *used;
> +
> +	if (!res->impl.addr_cmp)
> +		return -ENOSPC;
> +
> +	impl = (unsigned long *)&res->impl.addr_cmp;
> +	used = (unsigned long *)&res->used.addr_cmp;
> +	return etm4_cfg_find_unused_idx(impl, used, ETM_MAX_SINGLE_ADDR_CMP);
> +}
> +
> +
> +static inline int etm4_res_find_addr_comp_pair(struct cscfg_res_impl_used *res)
> +{
> +	unsigned long *impl, *used;
> +
> +	if (!res->impl.addr_cmp)
> +		return -ENOSPC;
> +
> +	impl = (unsigned long *)&res->impl.addr_cmp;
> +	used = (unsigned long *)&res->used.addr_cmp;
> +	return etm4_cfg_find_unused_idx_pair(impl, used, ETM_MAX_SINGLE_ADDR_CMP);
> +}
>  
>  #endif /* CORESIGHT_ETM4X_CFG_H */
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a348049ee08b..b513964b9305 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -223,6 +223,7 @@ struct coresight_sysfs_link {
>   * @feature_csdev_list: List of complex feature programming added to the device.
>   * @config_csdev_list:  List of system configurations added to the device.
>   * @active_cscfg_ctxt:  Context information for current active system configuration.
> + * @cscfg_res_mask:	Available device specific resources usable in features.
>   */
>  struct coresight_device {
>  	struct coresight_platform_data *pdata;
> @@ -248,6 +249,7 @@ struct coresight_device {
>  	struct list_head feature_csdev_list;
>  	struct list_head config_csdev_list;
>  	void *active_cscfg_ctxt;
> +	void *cscfg_res_mask;
>  };
>  
>  /*
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel