Re: [RFC PATCH v3 6/9] coresight: config: Add preloaded configurations

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

On Fri, Oct 30, 2020 at 05:56:52PM +0000, Mike Leach wrote:
> Preload set of configurations.
> 
> This patch creates a small set of preloaded configurations and features
> that are available immediately after coresight has been initialised.
> 
> The current set provides a strobing feature for ETMv4, that creates a
> periodic sampling of trace by switching trace generation on and off
> using counters in the ETM.
> 
> A configuration called "autofdo" is also provided that uses the 'strobing'
> feature and provides a couple of preset values, selectable on the perf
> command line.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/Makefile          |   3 +-
>  .../coresight/coresight-cfg-preload.c         | 160 ++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-core.c  |   6 +
>  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
>  4 files changed, 169 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-cfg-preload.c
> 
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index ea544206204d..9de5586cfd1a 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -4,7 +4,8 @@
>  #
>  obj-$(CONFIG_CORESIGHT) += coresight.o
>  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
> -		coresight-sysfs.o coresight-syscfg.o coresight-config.o
> +		coresight-sysfs.o coresight-syscfg.o coresight-config.o \
> +		coresight-cfg-preload.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
>  		      coresight-tmc-etr.o
> diff --git a/drivers/hwtracing/coresight/coresight-cfg-preload.c b/drivers/hwtracing/coresight/coresight-cfg-preload.c
> new file mode 100644
> index 000000000000..0975d64a1d9e
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cfg-preload.c

I think there needs to be a separation between the function of preloading
configurations and the preconfigurations themselves.  Ultimately having one .c
file for each preconfiguration would be optimal.  That will open the way for a
future feature where users get to select which preconfiguration they want to see
included.

> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(C) 2020 Linaro Limited. All rights reserved.
> + * Author: Mike Leach <mike.leach@linaro.org>
> + */
> +
> +#include "coresight-config.h"
> +#include "coresight-syscfg.h"
> +#include "coresight-etm4x-cfg.h"
> +
> +/* preload configurations and features */
> +
> +/* preload in features for ETMv4 */
> +
> +/* strobe feature */
> +
> +/* register defines */
> +#define STRB_REG_CTR (CS_CFG_REG_RESOURCE | ETM4_CFG_RES_CTR)
> +#define STRB_REG_CTR_RB (STRB_REG_CTR | CS_CFG_REG_VAL_SAVE)
> +#define STRB_REG_CTR_PRM (STRB_REG_CTR | CS_CFG_REG_VAL_PARAM)
> +#define STRB_REG_SEQ (CS_CFG_REG_RESOURCE | ETM4_CFG_RES_SEQ)
> +#define STRB_REG_SEL (CS_CFG_REG_RESOURCE | ETM4_CFG_RES_SEL)
> +#define STRB_REG_VI  (CS_CFG_REG_STD | CS_CFG_REG_VAL_MASK)
> +

This is the 3rd iteration of this set that I review.  Along the way I
have come to understand most of the things that are presented. The above and all
the information packed in .flags is one concept that did not get better as time
went by.  I have already remarked on the need to split the register address from
the flags themselves.  I think the remaining flags need further simplification.

Either some options are removed or a different way to present the information
is required.  Otherwise I fear people will not be able to write their own
preconfigurations. 

> +static struct cscfg_parameter_desc strobe_params[] = {
> +	{
> +		.name = "window",
> +		.value = 5000,
> +	},
> +	{
> +		.name = "period",
> +		.value = 10000,
> +	},
> +};
> +
> +static struct cscfg_regval strobe_regs[] = {
> +	/* resource selectors */
> +	{
> +		.flags = STRB_REG_SEL | TRCRSCTLRn(2),
> +		.val32 = 0x20001,
> +	},
> +	{
> +		.flags = STRB_REG_SEQ | TRCRSCTLRn(3),
> +		.val32 = 0x20002,
> +	},
> +	/* strobe window counter 0 - reload from param 0 */
> +	{
> +		.flags = STRB_REG_CTR_RB | TRCCNTVRn(0),
> +	},
> +	{
> +		.flags = STRB_REG_CTR_PRM | TRCCNTRLDVRn(0),
> +		.val32 = 0,
> +	},
> +	{
> +		.flags = STRB_REG_CTR | TRCCNTCTLRn(0),
> +		.val32 = 0x10001,
> +	},
> +	/* strobe period counter 1 - reload from param 1 */
> +	{
> +		.flags = STRB_REG_CTR_RB | TRCCNTVRn(1),
> +	},
> +	{
> +		.flags = STRB_REG_CTR_PRM | TRCCNTRLDVRn(1),
> +		.val32 = 1,
> +	},
> +	{
> +		.flags = STRB_REG_CTR | TRCCNTCTLRn(1),
> +		.val32 = 0x8102,
> +	},
> +	/* sequencer */
> +	{
> +		.flags = STRB_REG_SEQ | TRCSEQEVRn(0),
> +		.val32 = 0x0081,
> +	},
> +	{
> +		.flags = STRB_REG_SEQ | TRCSEQEVRn(1),
> +		.val32 = 0x0000,
> +	},
> +	/* view-inst */
> +	{
> +		.flags = STRB_REG_VI | TRCVICTLR,
> +		.val32 = 0x0003,
> +		.mask32 = 0x0003,
> +	},
> +	/* end of regs */
> +};
> +
> +static struct cscfg_feature_desc strobe = {
> +	.name = "strobing",
> +	.brief = "Generate periodic trace capture windows.\n"
> +	"parameter \'window\': a number of CPU cycles (W)\n"
> +	"parameter \'period\': trace enabled for W cycles every period x W cycles\n",
> +	.match_flags = CS_CFG_MATCH_CLASS_SRC_ETM4,
> +	.nr_params = ARRAY_SIZE(strobe_params),
> +	.params = strobe_params,
> +	.nr_regs = ARRAY_SIZE(strobe_regs),
> +	.regs = strobe_regs,
> +};
> +
> +static struct cscfg_feature_desc *preload_feats[] = {
> +	&strobe,
> +	0
> +};
> +
> +/* create an autofdo configuration */
> +
> +/* we will provide 9 sets of preset parameter values */
> +#define AFDO_NR_PRESETS		9
> +/* the total number of parameters in used features */
> +#define AFDO_NR_PARAM_SUM	ARRAY_SIZE(strobe_params)
> +
> +#define AFDO_MATCH_STROBING (CS_CFG_MATCH_INST_ANY | CS_CFG_MATCH_CLASS_SRC_ETM4)
> +
> +static struct cscfg_config_feat_ref afdo_refs[] = {
> +	{
> +		.name = "strobing",
> +		.nr_params = ARRAY_SIZE(strobe_params),
> +		.match = {
> +			.match_flags = AFDO_MATCH_STROBING,
> +		},
> +	},
> +};
> +
> +/*
> + * set of presets leaves strobing window constant while varying period to allow
> + * experimentation with mark / space ratios for various workloads
> + */
> +static u64 afdo_presets[AFDO_NR_PRESETS][AFDO_NR_PARAM_SUM] = {
> +	{ 5000, 2 },
> +	{ 5000, 4 },
> +	{ 5000, 8 },
> +	{ 5000, 16 },
> +	{ 5000, 64 },
> +	{ 5000, 128 },
> +	{ 5000, 512 },
> +	{ 5000, 1024 },
> +	{ 5000, 4096 },
> +};
> +
> +static struct cscfg_config_desc afdo = {
> +	.name = "autofdo",
> +	.brief = "Setup ETMs with strobing for autofdo\n"
> +	"Supplied presets allow experimentation with mark-space ratio for various loads\n",
> +	.nr_refs = ARRAY_SIZE(afdo_refs),
> +	.refs = afdo_refs,
> +	.nr_presets = AFDO_NR_PRESETS,
> +	.nr_total_params = AFDO_NR_PARAM_SUM,
> +	.presets = &afdo_presets[0][0],
> +};
> +

The current layout works if we have a single preconfiguration but doesn't scale
as soon as we need to add a new one.  I think the above should be in
coresight-afdo.c.  That way other preconfiguration can be added to
preload_cfgs[] without modifying anything else than the array declaration
itself. 

> +static struct cscfg_config_desc *preload_cfgs[] = {
> +	&afdo,
> +	0
> +};
> +
> +/* preload called with mutex locked */
> +int cscfg_preload(void)
> +{
> +	return cscfg_load_config_sets(preload_cfgs, preload_feats);
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 481d2b7b6b6f..dca84b3f5fca 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1680,9 +1680,15 @@ static int __init coresight_init(void)
>  
>  	/* initialise the coresight syscfg API */
>  	ret = cscfg_init();
> +	if (ret)
> +		goto exit_perf_close;
> +
> +	/* preload builtin configurations */
> +	ret = cscfg_preload();
>  	if (!ret)
>  		return 0;
>  
> +exit_perf_close:
>  	etm_perf_exit();
>  exit_bus_unregister:
>  	bus_unregister(&coresight_bustype);
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index ecf4aac7d712..e8f352599dd7 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -45,6 +45,7 @@ struct cscfg_csdev_register {
>  /* internal core operations for cscfg */
>  int __init cscfg_init(void);
>  void __exit cscfg_exit(void);
> +int cscfg_preload(void);
>  
>  /* syscfg external API */
>  int cscfg_load_config_sets(struct cscfg_config_desc **cfg_descs,
> -- 
> 2.17.1
> 

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