[RFC PATCH v3 6/9] coresight: config: Add preloaded configurations
Mike Leach
mike.leach at linaro.org
Thu Dec 24 14:21:33 EST 2020
Hi Mathieu,
On Tue, 24 Nov 2020 at 17:51, Mathieu Poirier
<mathieu.poirier at linaro.org> wrote:
>
> 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 at 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.
>
I think that the preloaded configs are going to be the exception
rather than the rule. The follow up sets will allow dynamic loading of
configurations, via loadable module or configfs.
This is the way people get what they want.
> > @@ -0,0 +1,160 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright(C) 2020 Linaro Limited. All rights reserved.
> > + * Author: Mike Leach <mike.leach at 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.
>
Agreed - flags are unclear - so from patch 1 the struct calls out the
usage of the values explicitly.
> 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.
>
I think that the change of reg flags to a struct that calls out the
useage, plus better docs should address this,.
> > +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.
>
Preload configs are both an example and a customer requirement driving this set.
Future may move towards choice via dynamic loading.
Thanks and Regards
Mile
> > +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
> >
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list