| Message ID | 20201030175655.30126-4-mike.leach@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CoreSight complex config support; ETM strobing | expand |
On Fri, Oct 30, 2020 at 05:56:49PM +0000, Mike Leach wrote: > Adds a set of generic support functions that allow devices to set and save > features values on the device, and enable and disable configurations. > > Additional functions for other common operations including feature > reset. > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/Makefile | 2 +- > .../hwtracing/coresight/coresight-config.c | 362 ++++++++++++++++++ > .../hwtracing/coresight/coresight-config.h | 14 + > .../hwtracing/coresight/coresight-syscfg.c | 4 + > 4 files changed, 381 insertions(+), 1 deletion(-) > create mode 100644 drivers/hwtracing/coresight/coresight-config.c > > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > index 4ce854c434b1..daad9f103a78 100644 > --- a/drivers/hwtracing/coresight/Makefile > +++ b/drivers/hwtracing/coresight/Makefile > @@ -4,7 +4,7 @@ > # > obj-$(CONFIG_CORESIGHT) += coresight.o > coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ > - coresight-sysfs.o coresight-syscfg.o > + coresight-sysfs.o coresight-syscfg.o coresight-config.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-config.c b/drivers/hwtracing/coresight/coresight-config.c > new file mode 100644 > index 000000000000..d911e0f083c1 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-config.c > @@ -0,0 +1,362 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright(C) 2020 Linaro Limited. All rights reserved. > + * Author: Mike Leach <mike.leach@linaro.org> > + */ > + > +#include <linux/sysfs.h> > +#include "coresight-config.h" > +#include "coresight-priv.h" > + > +/* > + * Write the value held in the register structure into the driver internal memory > + * location. > + */ > +static void cscfg_set_reg(struct cscfg_reg_csdev *reg) > +{ > + u32 *p_val32 = (u32 *)reg->drv_store; > + u32 tmp32; > + > + if (!(reg->value.flags & CS_CFG_REG_VAL_64BIT)) { > + if (reg->value.flags & CS_CFG_REG_VAL_MASK) { > + tmp32 = *p_val32; > + tmp32 &= ~reg->value.mask32; > + tmp32 |= reg->value.val32 & reg->value.mask32; > + *p_val32 = tmp32; > + } else > + *p_val32 = reg->value.val32; > + } else > + *((u64 *)reg->drv_store) = reg->value.val64; > +} > + > +/* > + * Read the driver value into the reg if this is marked as one we want to save. > + */ > +static void cscfg_save_reg(struct cscfg_reg_csdev *reg) > +{ > + if (!(reg->value.flags & CS_CFG_REG_VAL_SAVE)) > + return; > + if (reg->value.flags & CS_CFG_REG_VAL_64BIT) > + reg->value.val64 = *(u64 *)(reg->drv_store); > + else > + reg->value.val32 = *(u32 *)(reg->drv_store); > +} > + > +static inline void cscfg_init_reg_val(struct cscfg_reg_csdev *reg, > + const struct cscfg_regval *desc) > +{ > + reg->value.val64 = desc->val64; > +} > + > +static inline void cscfg_init_reg_flags(struct cscfg_reg_csdev *reg, > + const struct cscfg_regval *desc) > +{ > + reg->value.flags = desc->flags; > +} > + > +static void cscfg_init_reg_param(struct cscfg_parameter_csdev *param, > + struct cscfg_reg_csdev *reg) > +{ > + param->reg = reg; > + param->val64 = reg->value.flags & CS_CFG_REG_VAL_64BIT; > + > + if (param->val64) > + param->reg->value.val64 = param->current_value; > + else > + param->reg->value.val32 = (u32)param->current_value; > +} > + > +/* default set - will set values without resource checking */ > +static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat, const bool force_set) > +{ > + int i; > + > + spin_lock(feat->csdev_spinlock); > + if (feat->enabled || force_set) { > + for (i = 0; i < feat->nr_regs; i++) > + cscfg_set_reg(&feat->regs[i]); > + } > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > + feat->enabled || force_set ? "Set enabled" : "Skip disabled"); > + spin_unlock(feat->csdev_spinlock); > + return 0; > +} > + > +static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat, const bool force_save) > +{ > + int i; > + > + spin_lock(feat->csdev_spinlock); > + if (feat->enabled || force_save) { > + for (i = 0; i < feat->nr_regs; i++) > + cscfg_save_reg(&feat->regs[i]); > + } > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > + feat->enabled || force_save ? "Save disabled" : "Skip disabled"); > + spin_unlock(feat->csdev_spinlock); > +} > + > +/* default reset - restore default values, disable feature */ > +static void cscfg_reset_feat(struct cscfg_feature_csdev *feat) > +{ > + struct cscfg_reg_csdev *reg; > + struct cscfg_regval *reg_desc; > + struct cscfg_parameter_csdev *param; > + int i; > + > + spin_lock(feat->csdev_spinlock); > + feat->enabled = false; > + > + /* > + * set the default values for all parameters and regs from the > + * relevant static descriptors. > + */ > + for (i = 0; i < feat->nr_params; i++) > + feat->params[i].current_value = feat->desc->params[i].value; > + > + for (i = 0; i < feat->nr_regs; i++) { > + reg_desc = &feat->desc->regs[i]; > + reg = &feat->regs[i]; > + cscfg_init_reg_flags(reg, reg_desc); > + > + /* check if reg set from a parameter otherwise desc default */ > + if (reg_desc->flags & CS_CFG_REG_VAL_PARAM) { > + param = &feat->params[reg_desc->val32]; > + cscfg_init_reg_param(param, reg); > + } else > + cscfg_init_reg_val(reg, reg_desc); > + } > + spin_unlock(feat->csdev_spinlock); > +} > + > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat) > +{ > + feat->ops.set_on_enable = cscfg_set_on_enable; > + feat->ops.save_on_disable = cscfg_save_on_disable; > + feat->ops.reset = cscfg_reset_feat; > +} > + > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + int err = 0; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return 0; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + dev_dbg(&csdev->dev, "Found feature:%s", feat->desc->name); > + > + if (feat->ops.set_on_enable) { > + err = feat->ops.set_on_enable(feat, false); > + dev_dbg(&csdev->dev, "Feature %s: %s", > + feat->desc->name, err ? "Set failed" : "OK"); > + if (!err) > + break; > + } > + } > + spin_unlock(&csdev->cfg_lock); > + return err; > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_set_enabled_feats); > + > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + if (feat->ops.save_on_disable) > + feat->ops.save_on_disable(feat, false); > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_save_enabled_feats); > + > +void cscfg_csdev_reset_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + if (feat->ops.reset) > + feat->ops.reset(feat); > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats); > + > +static int cscfg_update_presets(struct cscfg_config_csdev *cfg, int preset) > +{ > + int i, j, line_offset = 0, val_idx = 0, max_idx; > + u64 val; > + struct cscfg_feature_csdev *feat; > + struct cscfg_parameter_csdev *param; > + const char *name; int i, j, line_offset = 0, val_idx = 0, max_idx; struct cscfg_parameter_csdev *param; struct cscfg_feature_csdev *feat; const char *name; u64 val; > + > + if (preset > cfg->desc->nr_presets) Extra space between if and () > + return -EINVAL; > + /* > + * Go through the array of features, assigning preset values to > + * feature parameters in the order they appear. > + * There should be precisely the same number of preset values as the > + * sum of number of parameters over all the features - but we will > + * ensure there is no overrun. > + */ > + line_offset = (preset-1) * cfg->desc->nr_total_params; If I understand correctly this computation won't work in situations where there is more than one feature with varying number of parameters. > + max_idx = cfg->desc->nr_total_params; > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + if (feat->nr_params) { if (!feat->nr_params) continue; > + spin_lock(feat->csdev_spinlock); > + for (j = 0; j < feat->nr_params; j++) { > + param = &feat->params[j]; > + name = feat->desc->params[j].name; > + val = cfg->desc->presets[line_offset + val_idx++]; > + if (param->val64) { > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%lld)", name, val); > + param->reg->value.val64 = val; > + } else { > + param->reg->value.val32 = (u32)val; > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%d)", name, (u32)val); > + } > + if (val_idx >= max_idx) > + break; > + } > + spin_unlock(feat->csdev_spinlock); > + } > + > + /* don't overrun the preset array line */ > + if (val_idx >= max_idx) > + break; > + } > + return 0; > +} > + > +/* > + * if we are not using a preset, then need to update the feature params > + * with current values. > + */ > +static int cscfg_update_curr_params(struct cscfg_config_csdev *cfg) > +{ > + int i, j; > + struct cscfg_feature_csdev *feat; > + struct cscfg_parameter_csdev *param; > + const char *name; > + u64 val; > + > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + if (feat->nr_params) { if (!feat->nr_params) continue; > + spin_lock(feat->csdev_spinlock); Please add a comment on why taking the spinlock is needed, i.e what kind of action would corrupt the data. > + for (j = 0; j < feat->nr_params; j++) { > + param = &feat->params[j]; > + name = feat->desc->params[j].name; > + val = param->current_value; > + if (param->val64) { > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%lld)", name, val); > + param->reg->value.val64 = val; > + } else { > + param->reg->value.val32 = (u32)val; > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%d)", name, (u32)val); > + } > + } > + spin_unlock(feat->csdev_spinlock); > + } > + } > + return 0; > +} > + > +static int cscfg_prog_config(struct cscfg_config_csdev *cfg, bool enable) > +{ > + int i, err = 0; > + struct cscfg_feature_csdev *feat; > + struct coresight_device *csdev; > + > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + csdev = feat->csdev; > + dev_dbg(&csdev->dev, "cfg %s; %s feature:%s", cfg->desc->name, > + enable ? "enable" : "disable", feat->desc->name); > + > + if (enable) { > + if (feat->ops.set_on_enable) > + err = feat->ops.set_on_enable(feat, true); > + } else { > + if (feat->ops.save_on_disable) > + feat->ops.save_on_disable(feat, true); > + } > + if (err) > + break; > + } > + return err; > +} > + > +/** > + * Enable configuration for the device. > + * Match the config id and optionally set preset values for parameters. > + * > + * @csdev: coresight device to set config on. > + * @cfg_id: hash id of configuration. > + * @preset: preset values to use - 0 for default. > + */ > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > + unsigned long cfg_id, > + int preset) Please avoid stacking when possible. Here @preset fits on the previous line. The same applies for the function declaration in coresigt-config.h I will continue with this patch tomorrow. Thanks, Mathieu > +{ > + struct cscfg_config_csdev *cfg; > + int err = 0; > + > + dev_dbg(&csdev->dev, "Check for config %lx, preset %d", cfg_id, preset); > + > + spin_lock(&csdev->cfg_lock); > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > + dev_dbg(&csdev->dev, "checking %s", cfg->desc->name); > + if (cfg->id_hash == cfg_id) { > + if (preset) > + err = cscfg_update_presets(cfg, preset); > + else > + err = cscfg_update_curr_params(cfg); > + if (!err) > + err = cscfg_prog_config(cfg, true); > + if (!err) > + cfg->enabled = true; > + break; > + } > + } > + spin_unlock(&csdev->cfg_lock); > + return err; > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_enable_config); > + > +void cscfg_csdev_disable_config(struct coresight_device *csdev) > +{ > + struct cscfg_config_csdev *cfg; > + > + spin_lock(&csdev->cfg_lock); > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > + if (cfg->enabled) { > + cscfg_prog_config(cfg, false); > + cfg->enabled = false; > + } > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_disable_config); > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h > index 8fd7ff991ced..a3d374ebb70f 100644 > --- a/drivers/hwtracing/coresight/coresight-config.h > +++ b/drivers/hwtracing/coresight/coresight-config.h > @@ -287,4 +287,18 @@ struct cscfg_csdev_feat_ops { > struct cscfg_feature_csdev *feat); > }; > > +/* coresight config API functions */ > + > +/* helper functions for feature manipulation */ > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat); > + > +/* enable / disable features or configs on a device */ > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev); > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev); > +void cscfg_csdev_reset_feats(struct coresight_device *csdev); > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > + unsigned long cfg_id, > + int preset); > +void cscfg_csdev_disable_config(struct coresight_device *csdev); > + > #endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 53bc1d551171..98ee78e3412a 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -166,6 +166,7 @@ cscfg_alloc_csdev_feat(struct coresight_device *csdev, struct cscfg_feature_desc > feat->csdev = csdev; > for (i = 0; i < feat->nr_params; i++) > feat->params[i].feat = feat; > + cscfg_set_def_ops(feat); > > return feat; > } > @@ -195,6 +196,9 @@ static int cscfg_load_feat_csdev(struct coresight_device *csdev, > list_add(&feat_csdev->node, &csdev->feat_list); > spin_unlock(&csdev->cfg_lock); > > + /* force load of default parameter values into registers */ > + feat_csdev->ops.reset(feat_csdev); > + > return 0; > } > > -- > 2.17.1 >
Hi Mike, On Fri, Oct 30, 2020 at 05:56:49PM +0000, Mike Leach wrote: > Adds a set of generic support functions that allow devices to set and save > features values on the device, and enable and disable configurations. > > Additional functions for other common operations including feature > reset. > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/Makefile | 2 +- > .../hwtracing/coresight/coresight-config.c | 362 ++++++++++++++++++ > .../hwtracing/coresight/coresight-config.h | 14 + > .../hwtracing/coresight/coresight-syscfg.c | 4 + > 4 files changed, 381 insertions(+), 1 deletion(-) > create mode 100644 drivers/hwtracing/coresight/coresight-config.c > > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > index 4ce854c434b1..daad9f103a78 100644 > --- a/drivers/hwtracing/coresight/Makefile > +++ b/drivers/hwtracing/coresight/Makefile > @@ -4,7 +4,7 @@ > # > obj-$(CONFIG_CORESIGHT) += coresight.o > coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ > - coresight-sysfs.o coresight-syscfg.o > + coresight-sysfs.o coresight-syscfg.o coresight-config.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-config.c b/drivers/hwtracing/coresight/coresight-config.c > new file mode 100644 > index 000000000000..d911e0f083c1 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-config.c > @@ -0,0 +1,362 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright(C) 2020 Linaro Limited. All rights reserved. > + * Author: Mike Leach <mike.leach@linaro.org> > + */ > + > +#include <linux/sysfs.h> > +#include "coresight-config.h" > +#include "coresight-priv.h" > + > +/* > + * Write the value held in the register structure into the driver internal memory > + * location. > + */ > +static void cscfg_set_reg(struct cscfg_reg_csdev *reg) > +{ > + u32 *p_val32 = (u32 *)reg->drv_store; > + u32 tmp32; > + > + if (!(reg->value.flags & CS_CFG_REG_VAL_64BIT)) { > + if (reg->value.flags & CS_CFG_REG_VAL_MASK) { > + tmp32 = *p_val32; > + tmp32 &= ~reg->value.mask32; > + tmp32 |= reg->value.val32 & reg->value.mask32; > + *p_val32 = tmp32; > + } else > + *p_val32 = reg->value.val32; > + } else > + *((u64 *)reg->drv_store) = reg->value.val64; > +} > + > +/* > + * Read the driver value into the reg if this is marked as one we want to save. > + */ > +static void cscfg_save_reg(struct cscfg_reg_csdev *reg) > +{ > + if (!(reg->value.flags & CS_CFG_REG_VAL_SAVE)) > + return; > + if (reg->value.flags & CS_CFG_REG_VAL_64BIT) > + reg->value.val64 = *(u64 *)(reg->drv_store); > + else > + reg->value.val32 = *(u32 *)(reg->drv_store); > +} > + > +static inline void cscfg_init_reg_val(struct cscfg_reg_csdev *reg, > + const struct cscfg_regval *desc) > +{ > + reg->value.val64 = desc->val64; > +} > + > +static inline void cscfg_init_reg_flags(struct cscfg_reg_csdev *reg, > + const struct cscfg_regval *desc) > +{ > + reg->value.flags = desc->flags; > +} > + > +static void cscfg_init_reg_param(struct cscfg_parameter_csdev *param, > + struct cscfg_reg_csdev *reg) > +{ > + param->reg = reg; > + param->val64 = reg->value.flags & CS_CFG_REG_VAL_64BIT; > + > + if (param->val64) > + param->reg->value.val64 = param->current_value; > + else > + param->reg->value.val32 = (u32)param->current_value; > +} > + > +/* default set - will set values without resource checking */ > +static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat, const bool force_set) > +{ Please add a description of what @force_set is used for and why we need it. Same for @force_save in cscfg_save_on_disable() below. > + int i; > + > + spin_lock(feat->csdev_spinlock); > + if (feat->enabled || force_set) { > + for (i = 0; i < feat->nr_regs; i++) > + cscfg_set_reg(&feat->regs[i]); > + } > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > + feat->enabled || force_set ? "Set enabled" : "Skip disabled"); > + spin_unlock(feat->csdev_spinlock); > + return 0; > +} > + > +static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat, const bool force_save) > +{ > + int i; > + > + spin_lock(feat->csdev_spinlock); > + if (feat->enabled || force_save) { > + for (i = 0; i < feat->nr_regs; i++) > + cscfg_save_reg(&feat->regs[i]); > + } > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > + feat->enabled || force_save ? "Save disabled" : "Skip disabled"); > + spin_unlock(feat->csdev_spinlock); > +} > + > +/* default reset - restore default values, disable feature */ > +static void cscfg_reset_feat(struct cscfg_feature_csdev *feat) > +{ > + struct cscfg_reg_csdev *reg; struct cscfg_reg_csdev *reg_csdev; > + struct cscfg_regval *reg_desc; Renaming struct cscfg_regval to cscfg_reg_desc would be consistent with what is done for parameters, features and configurations. The above would then become: struct cscfg_reg_desc *reg_desc; And that is very easy to remember when reading the code. I suggest that all instances of these structures (thoughout the patchset) follow this convention. > + struct cscfg_parameter_csdev *param; Here too, and throughout: struct cscfg_parameter_csdev *param_csdev; > + int i; > + > + spin_lock(feat->csdev_spinlock); > + feat->enabled = false; > + > + /* > + * set the default values for all parameters and regs from the > + * relevant static descriptors. > + */ > + for (i = 0; i < feat->nr_params; i++) > + feat->params[i].current_value = feat->desc->params[i].value; > + > + for (i = 0; i < feat->nr_regs; i++) { > + reg_desc = &feat->desc->regs[i]; > + reg = &feat->regs[i]; > + cscfg_init_reg_flags(reg, reg_desc); I would just initialise the flag, no need for another function. > + > + /* check if reg set from a parameter otherwise desc default */ > + if (reg_desc->flags & CS_CFG_REG_VAL_PARAM) { > + param = &feat->params[reg_desc->val32]; There is something wrong with reg_desc->val32 as an array index in the above. I'm surprised it did blow up on you yet. Moreover this is another instance where having multiple features, each with their own number of parameters, will be complex to work with. > + cscfg_init_reg_param(param, reg); > + } else > + cscfg_init_reg_val(reg, reg_desc); Here too I wouldn't bother with a function. > + } > + spin_unlock(feat->csdev_spinlock); > +} > + > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat) > +{ > + feat->ops.set_on_enable = cscfg_set_on_enable; > + feat->ops.save_on_disable = cscfg_save_on_disable; > + feat->ops.reset = cscfg_reset_feat; > +} When do you expect these to change, at least for this set? For the time being I am done with this patch. Thanks, Mathieu > + > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + int err = 0; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return 0; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + dev_dbg(&csdev->dev, "Found feature:%s", feat->desc->name); > + > + if (feat->ops.set_on_enable) { > + err = feat->ops.set_on_enable(feat, false); > + dev_dbg(&csdev->dev, "Feature %s: %s", > + feat->desc->name, err ? "Set failed" : "OK"); > + if (!err) > + break; > + } > + } > + spin_unlock(&csdev->cfg_lock); > + return err; > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_set_enabled_feats); > + > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + if (feat->ops.save_on_disable) > + feat->ops.save_on_disable(feat, false); > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_save_enabled_feats); > + > +void cscfg_csdev_reset_feats(struct coresight_device *csdev) > +{ > + struct cscfg_feature_csdev *feat; > + > + spin_lock(&csdev->cfg_lock); > + if (list_empty(&csdev->feat_list)) { > + spin_unlock(&csdev->cfg_lock); > + return; > + } > + > + list_for_each_entry(feat, &csdev->feat_list, node) { > + if (feat->ops.reset) > + feat->ops.reset(feat); > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats); > + > +static int cscfg_update_presets(struct cscfg_config_csdev *cfg, int preset) > +{ > + int i, j, line_offset = 0, val_idx = 0, max_idx; > + u64 val; > + struct cscfg_feature_csdev *feat; > + struct cscfg_parameter_csdev *param; > + const char *name; > + > + if (preset > cfg->desc->nr_presets) > + return -EINVAL; > + /* > + * Go through the array of features, assigning preset values to > + * feature parameters in the order they appear. > + * There should be precisely the same number of preset values as the > + * sum of number of parameters over all the features - but we will > + * ensure there is no overrun. > + */ > + line_offset = (preset-1) * cfg->desc->nr_total_params; > + max_idx = cfg->desc->nr_total_params; > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + if (feat->nr_params) { > + spin_lock(feat->csdev_spinlock); > + for (j = 0; j < feat->nr_params; j++) { > + param = &feat->params[j]; > + name = feat->desc->params[j].name; > + val = cfg->desc->presets[line_offset + val_idx++]; > + if (param->val64) { > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%lld)", name, val); > + param->reg->value.val64 = val; > + } else { > + param->reg->value.val32 = (u32)val; > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%d)", name, (u32)val); > + } > + if (val_idx >= max_idx) > + break; > + } > + spin_unlock(feat->csdev_spinlock); > + } > + > + /* don't overrun the preset array line */ > + if (val_idx >= max_idx) > + break; > + } > + return 0; > +} > + > +/* > + * if we are not using a preset, then need to update the feature params > + * with current values. > + */ > +static int cscfg_update_curr_params(struct cscfg_config_csdev *cfg) > +{ > + int i, j; > + struct cscfg_feature_csdev *feat; > + struct cscfg_parameter_csdev *param; > + const char *name; > + u64 val; > + > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + if (feat->nr_params) { > + spin_lock(feat->csdev_spinlock); > + for (j = 0; j < feat->nr_params; j++) { > + param = &feat->params[j]; > + name = feat->desc->params[j].name; > + val = param->current_value; > + if (param->val64) { > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%lld)", name, val); > + param->reg->value.val64 = val; > + } else { > + param->reg->value.val32 = (u32)val; > + dev_dbg(&cfg->csdev->dev, > + "set param %s (%d)", name, (u32)val); > + } > + } > + spin_unlock(feat->csdev_spinlock); > + } > + } > + return 0; > +} > + > +static int cscfg_prog_config(struct cscfg_config_csdev *cfg, bool enable) > +{ > + int i, err = 0; > + struct cscfg_feature_csdev *feat; > + struct coresight_device *csdev; > + > + for (i = 0; i < cfg->nr_feat; i++) { > + feat = cfg->feats[i]; > + csdev = feat->csdev; > + dev_dbg(&csdev->dev, "cfg %s; %s feature:%s", cfg->desc->name, > + enable ? "enable" : "disable", feat->desc->name); > + > + if (enable) { > + if (feat->ops.set_on_enable) > + err = feat->ops.set_on_enable(feat, true); > + } else { > + if (feat->ops.save_on_disable) > + feat->ops.save_on_disable(feat, true); > + } > + if (err) > + break; > + } > + return err; > +} > + > +/** > + * Enable configuration for the device. > + * Match the config id and optionally set preset values for parameters. > + * > + * @csdev: coresight device to set config on. > + * @cfg_id: hash id of configuration. > + * @preset: preset values to use - 0 for default. > + */ > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > + unsigned long cfg_id, > + int preset) > +{ > + struct cscfg_config_csdev *cfg; > + int err = 0; > + > + dev_dbg(&csdev->dev, "Check for config %lx, preset %d", cfg_id, preset); > + > + spin_lock(&csdev->cfg_lock); > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > + dev_dbg(&csdev->dev, "checking %s", cfg->desc->name); > + if (cfg->id_hash == cfg_id) { > + if (preset) > + err = cscfg_update_presets(cfg, preset); > + else > + err = cscfg_update_curr_params(cfg); > + if (!err) > + err = cscfg_prog_config(cfg, true); > + if (!err) > + cfg->enabled = true; > + break; > + } > + } > + spin_unlock(&csdev->cfg_lock); > + return err; > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_enable_config); > + > +void cscfg_csdev_disable_config(struct coresight_device *csdev) > +{ > + struct cscfg_config_csdev *cfg; > + > + spin_lock(&csdev->cfg_lock); > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > + if (cfg->enabled) { > + cscfg_prog_config(cfg, false); > + cfg->enabled = false; > + } > + } > + spin_unlock(&csdev->cfg_lock); > +} > +EXPORT_SYMBOL_GPL(cscfg_csdev_disable_config); > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h > index 8fd7ff991ced..a3d374ebb70f 100644 > --- a/drivers/hwtracing/coresight/coresight-config.h > +++ b/drivers/hwtracing/coresight/coresight-config.h > @@ -287,4 +287,18 @@ struct cscfg_csdev_feat_ops { > struct cscfg_feature_csdev *feat); > }; > > +/* coresight config API functions */ > + > +/* helper functions for feature manipulation */ > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat); > + > +/* enable / disable features or configs on a device */ > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev); > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev); > +void cscfg_csdev_reset_feats(struct coresight_device *csdev); > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > + unsigned long cfg_id, > + int preset); > +void cscfg_csdev_disable_config(struct coresight_device *csdev); > + > #endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 53bc1d551171..98ee78e3412a 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -166,6 +166,7 @@ cscfg_alloc_csdev_feat(struct coresight_device *csdev, struct cscfg_feature_desc > feat->csdev = csdev; > for (i = 0; i < feat->nr_params; i++) > feat->params[i].feat = feat; > + cscfg_set_def_ops(feat); > > return feat; > } > @@ -195,6 +196,9 @@ static int cscfg_load_feat_csdev(struct coresight_device *csdev, > list_add(&feat_csdev->node, &csdev->feat_list); > spin_unlock(&csdev->cfg_lock); > > + /* force load of default parameter values into registers */ > + feat_csdev->ops.reset(feat_csdev); > + > return 0; > } > > -- > 2.17.1 >
Hi Mathieu, From the first set of comments " > + line_offset = (preset-1) * cfg->desc->nr_total_params; If I understand correctly this computation won't work in situations where there is more than one feature with varying number of parameters. " This is correct - desc->nr_total_params is defined as the sum of all feat->nr_params for the config. Hence this is the length of a row in the preset array. On Thu, 19 Nov 2020 at 22:29, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > Hi Mike, > > On Fri, Oct 30, 2020 at 05:56:49PM +0000, Mike Leach wrote: > > Adds a set of generic support functions that allow devices to set and save > > features values on the device, and enable and disable configurations. > > > > Additional functions for other common operations including feature > > reset. > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > --- > > drivers/hwtracing/coresight/Makefile | 2 +- > > .../hwtracing/coresight/coresight-config.c | 362 ++++++++++++++++++ > > .../hwtracing/coresight/coresight-config.h | 14 + > > .../hwtracing/coresight/coresight-syscfg.c | 4 + > > 4 files changed, 381 insertions(+), 1 deletion(-) > > create mode 100644 drivers/hwtracing/coresight/coresight-config.c > > > > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > > index 4ce854c434b1..daad9f103a78 100644 > > --- a/drivers/hwtracing/coresight/Makefile > > +++ b/drivers/hwtracing/coresight/Makefile > > @@ -4,7 +4,7 @@ > > # > > obj-$(CONFIG_CORESIGHT) += coresight.o > > coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ > > - coresight-sysfs.o coresight-syscfg.o > > + coresight-sysfs.o coresight-syscfg.o coresight-config.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-config.c b/drivers/hwtracing/coresight/coresight-config.c > > new file mode 100644 > > index 000000000000..d911e0f083c1 > > --- /dev/null > > +++ b/drivers/hwtracing/coresight/coresight-config.c > > @@ -0,0 +1,362 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright(C) 2020 Linaro Limited. All rights reserved. > > + * Author: Mike Leach <mike.leach@linaro.org> > > + */ > > + > > +#include <linux/sysfs.h> > > +#include "coresight-config.h" > > +#include "coresight-priv.h" > > + > > +/* > > + * Write the value held in the register structure into the driver internal memory > > + * location. > > + */ > > +static void cscfg_set_reg(struct cscfg_reg_csdev *reg) > > +{ > > + u32 *p_val32 = (u32 *)reg->drv_store; > > + u32 tmp32; > > + > > + if (!(reg->value.flags & CS_CFG_REG_VAL_64BIT)) { > > + if (reg->value.flags & CS_CFG_REG_VAL_MASK) { > > + tmp32 = *p_val32; > > + tmp32 &= ~reg->value.mask32; > > + tmp32 |= reg->value.val32 & reg->value.mask32; > > + *p_val32 = tmp32; > > + } else > > + *p_val32 = reg->value.val32; > > + } else > > + *((u64 *)reg->drv_store) = reg->value.val64; > > +} > > + > > +/* > > + * Read the driver value into the reg if this is marked as one we want to save. > > + */ > > +static void cscfg_save_reg(struct cscfg_reg_csdev *reg) > > +{ > > + if (!(reg->value.flags & CS_CFG_REG_VAL_SAVE)) > > + return; > > + if (reg->value.flags & CS_CFG_REG_VAL_64BIT) > > + reg->value.val64 = *(u64 *)(reg->drv_store); > > + else > > + reg->value.val32 = *(u32 *)(reg->drv_store); > > +} > > + > > +static inline void cscfg_init_reg_val(struct cscfg_reg_csdev *reg, > > + const struct cscfg_regval *desc) > > +{ > > + reg->value.val64 = desc->val64; > > +} > > + > > +static inline void cscfg_init_reg_flags(struct cscfg_reg_csdev *reg, > > + const struct cscfg_regval *desc) > > +{ > > + reg->value.flags = desc->flags; > > +} > > + > > +static void cscfg_init_reg_param(struct cscfg_parameter_csdev *param, > > + struct cscfg_reg_csdev *reg) > > +{ > > + param->reg = reg; > > + param->val64 = reg->value.flags & CS_CFG_REG_VAL_64BIT; > > + > > + if (param->val64) > > + param->reg->value.val64 = param->current_value; > > + else > > + param->reg->value.val32 = (u32)param->current_value; > > +} > > + > > +/* default set - will set values without resource checking */ > > +static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat, const bool force_set) > > +{ > > Please add a description of what @force_set is used for and why we need it. > Same for @force_save in cscfg_save_on_disable() below. > This is changing considerably. I was originally trying to account for features being enabled independently from configs - in a similar way we can use perf and sysfs independently - but this is unneeded and too complicated. Moving forwards enabling a config, enables dependent features - so the @force thing is dropped for an updated mechanism. > > + int i; > > + > > + spin_lock(feat->csdev_spinlock); > > + if (feat->enabled || force_set) { > > + for (i = 0; i < feat->nr_regs; i++) > > + cscfg_set_reg(&feat->regs[i]); > > + } > > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > > + feat->enabled || force_set ? "Set enabled" : "Skip disabled"); > > + spin_unlock(feat->csdev_spinlock); > > + return 0; > > +} > > + > > +static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat, const bool force_save) > > +{ > > + int i; > > + > > + spin_lock(feat->csdev_spinlock); > > + if (feat->enabled || force_save) { > > + for (i = 0; i < feat->nr_regs; i++) > > + cscfg_save_reg(&feat->regs[i]); > > + } > > + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, > > + feat->enabled || force_save ? "Save disabled" : "Skip disabled"); > > + spin_unlock(feat->csdev_spinlock); > > +} > > + > > +/* default reset - restore default values, disable feature */ > > +static void cscfg_reset_feat(struct cscfg_feature_csdev *feat) > > +{ > > + struct cscfg_reg_csdev *reg; > > struct cscfg_reg_csdev *reg_csdev; > > > + struct cscfg_regval *reg_desc; > > Renaming struct cscfg_regval to cscfg_reg_desc would be consistent with what is > done for parameters, features and configurations. The above would then become: > > struct cscfg_reg_desc *reg_desc; > > And that is very easy to remember when reading the code. I suggest that all > instances of these structures (thoughout the patchset) follow this convention. > > > + struct cscfg_parameter_csdev *param; > > Here too, and throughout: > > struct cscfg_parameter_csdev *param_csdev; > Agreed - convention works well.] > > + int i; > > + > > + spin_lock(feat->csdev_spinlock); > > + feat->enabled = false; > > + > > + /* > > + * set the default values for all parameters and regs from the > > + * relevant static descriptors. > > + */ > > + for (i = 0; i < feat->nr_params; i++) > > + feat->params[i].current_value = feat->desc->params[i].value; > > + > > + for (i = 0; i < feat->nr_regs; i++) { > > + reg_desc = &feat->desc->regs[i]; > > + reg = &feat->regs[i]; > > + cscfg_init_reg_flags(reg, reg_desc); > > I would just initialise the flag, no need for another function. > > > + > > + /* check if reg set from a parameter otherwise desc default */ > > + if (reg_desc->flags & CS_CFG_REG_VAL_PARAM) { > > + param = &feat->params[reg_desc->val32]; > > There is something wrong with reg_desc->val32 as an array index in the above. > I'm surprised it did blow up on you yet. Moreover this is another instance > where having multiple features, each with their own number of parameters, will > be complex to work with. > If the regcal_desc defines the value as a parameter index, then this is fine. However, there is a case for range validation. > > + cscfg_init_reg_param(param, reg); > > + } else > > + cscfg_init_reg_val(reg, reg_desc); > > Here too I wouldn't bother with a function. > I was trying for better readability - plus the init of reg can depend on flags in reg_desc. > > + } > > + spin_unlock(feat->csdev_spinlock); > > +} > > + > > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat) > > +{ > > + feat->ops.set_on_enable = cscfg_set_on_enable; > > + feat->ops.save_on_disable = cscfg_save_on_disable; > > + feat->ops.reset = cscfg_reset_feat; > > +} > > When do you expect these to change, at least for this set? > Nowhere in this set, but as support is expanded, device specific elements will appear in set on enable - specifically in ETM, where the register is defined as a resource e.g. counter N. so the ETMv4 driver will override this to select an appropriate counter. Thus the feature says "I want this to be a counter resource", and on set, the ETM driver will pick an unused counter. This is for the follow up sets, but I wanted to establish the API here. Thanks Mike > For the time being I am done with this patch. > > Thanks, > Mathieu > > > + > > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev) > > +{ > > + struct cscfg_feature_csdev *feat; > > + int err = 0; > > + > > + spin_lock(&csdev->cfg_lock); > > + if (list_empty(&csdev->feat_list)) { > > + spin_unlock(&csdev->cfg_lock); > > + return 0; > > + } > > + > > + list_for_each_entry(feat, &csdev->feat_list, node) { > > + dev_dbg(&csdev->dev, "Found feature:%s", feat->desc->name); > > + > > + if (feat->ops.set_on_enable) { > > + err = feat->ops.set_on_enable(feat, false); > > + dev_dbg(&csdev->dev, "Feature %s: %s", > > + feat->desc->name, err ? "Set failed" : "OK"); > > + if (!err) > > + break; > > + } > > + } > > + spin_unlock(&csdev->cfg_lock); > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(cscfg_csdev_set_enabled_feats); > > + > > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev) > > +{ > > + struct cscfg_feature_csdev *feat; > > + > > + spin_lock(&csdev->cfg_lock); > > + if (list_empty(&csdev->feat_list)) { > > + spin_unlock(&csdev->cfg_lock); > > + return; > > + } > > + > > + list_for_each_entry(feat, &csdev->feat_list, node) { > > + if (feat->ops.save_on_disable) > > + feat->ops.save_on_disable(feat, false); > > + } > > + spin_unlock(&csdev->cfg_lock); > > +} > > +EXPORT_SYMBOL_GPL(cscfg_csdev_save_enabled_feats); > > + > > +void cscfg_csdev_reset_feats(struct coresight_device *csdev) > > +{ > > + struct cscfg_feature_csdev *feat; > > + > > + spin_lock(&csdev->cfg_lock); > > + if (list_empty(&csdev->feat_list)) { > > + spin_unlock(&csdev->cfg_lock); > > + return; > > + } > > + > > + list_for_each_entry(feat, &csdev->feat_list, node) { > > + if (feat->ops.reset) > > + feat->ops.reset(feat); > > + } > > + spin_unlock(&csdev->cfg_lock); > > +} > > +EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats); > > + > > +static int cscfg_update_presets(struct cscfg_config_csdev *cfg, int preset) > > +{ > > + int i, j, line_offset = 0, val_idx = 0, max_idx; > > + u64 val; > > + struct cscfg_feature_csdev *feat; > > + struct cscfg_parameter_csdev *param; > > + const char *name; > > + > > + if (preset > cfg->desc->nr_presets) > > + return -EINVAL; > > + /* > > + * Go through the array of features, assigning preset values to > > + * feature parameters in the order they appear. > > + * There should be precisely the same number of preset values as the > > + * sum of number of parameters over all the features - but we will > > + * ensure there is no overrun. > > + */ > > + line_offset = (preset-1) * cfg->desc->nr_total_params; > > + max_idx = cfg->desc->nr_total_params; > > + for (i = 0; i < cfg->nr_feat; i++) { > > + feat = cfg->feats[i]; > > + if (feat->nr_params) { > > + spin_lock(feat->csdev_spinlock); > > + for (j = 0; j < feat->nr_params; j++) { > > + param = &feat->params[j]; > > + name = feat->desc->params[j].name; > > + val = cfg->desc->presets[line_offset + val_idx++]; > > + if (param->val64) { > > + dev_dbg(&cfg->csdev->dev, > > + "set param %s (%lld)", name, val); > > + param->reg->value.val64 = val; > > + } else { > > + param->reg->value.val32 = (u32)val; > > + dev_dbg(&cfg->csdev->dev, > > + "set param %s (%d)", name, (u32)val); > > + } > > + if (val_idx >= max_idx) > > + break; > > + } > > + spin_unlock(feat->csdev_spinlock); > > + } > > + > > + /* don't overrun the preset array line */ > > + if (val_idx >= max_idx) > > + break; > > + } > > + return 0; > > +} > > + > > +/* > > + * if we are not using a preset, then need to update the feature params > > + * with current values. > > + */ > > +static int cscfg_update_curr_params(struct cscfg_config_csdev *cfg) > > +{ > > + int i, j; > > + struct cscfg_feature_csdev *feat; > > + struct cscfg_parameter_csdev *param; > > + const char *name; > > + u64 val; > > + > > + for (i = 0; i < cfg->nr_feat; i++) { > > + feat = cfg->feats[i]; > > + if (feat->nr_params) { > > + spin_lock(feat->csdev_spinlock); > > + for (j = 0; j < feat->nr_params; j++) { > > + param = &feat->params[j]; > > + name = feat->desc->params[j].name; > > + val = param->current_value; > > + if (param->val64) { > > + dev_dbg(&cfg->csdev->dev, > > + "set param %s (%lld)", name, val); > > + param->reg->value.val64 = val; > > + } else { > > + param->reg->value.val32 = (u32)val; > > + dev_dbg(&cfg->csdev->dev, > > + "set param %s (%d)", name, (u32)val); > > + } > > + } > > + spin_unlock(feat->csdev_spinlock); > > + } > > + } > > + return 0; > > +} > > + > > +static int cscfg_prog_config(struct cscfg_config_csdev *cfg, bool enable) > > +{ > > + int i, err = 0; > > + struct cscfg_feature_csdev *feat; > > + struct coresight_device *csdev; > > + > > + for (i = 0; i < cfg->nr_feat; i++) { > > + feat = cfg->feats[i]; > > + csdev = feat->csdev; > > + dev_dbg(&csdev->dev, "cfg %s; %s feature:%s", cfg->desc->name, > > + enable ? "enable" : "disable", feat->desc->name); > > + > > + if (enable) { > > + if (feat->ops.set_on_enable) > > + err = feat->ops.set_on_enable(feat, true); > > + } else { > > + if (feat->ops.save_on_disable) > > + feat->ops.save_on_disable(feat, true); > > + } > > + if (err) > > + break; > > + } > > + return err; > > +} > > + > > +/** > > + * Enable configuration for the device. > > + * Match the config id and optionally set preset values for parameters. > > + * > > + * @csdev: coresight device to set config on. > > + * @cfg_id: hash id of configuration. > > + * @preset: preset values to use - 0 for default. > > + */ > > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > > + unsigned long cfg_id, > > + int preset) > > +{ > > + struct cscfg_config_csdev *cfg; > > + int err = 0; > > + > > + dev_dbg(&csdev->dev, "Check for config %lx, preset %d", cfg_id, preset); > > + > > + spin_lock(&csdev->cfg_lock); > > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > > + dev_dbg(&csdev->dev, "checking %s", cfg->desc->name); > > + if (cfg->id_hash == cfg_id) { > > + if (preset) > > + err = cscfg_update_presets(cfg, preset); > > + else > > + err = cscfg_update_curr_params(cfg); > > + if (!err) > > + err = cscfg_prog_config(cfg, true); > > + if (!err) > > + cfg->enabled = true; > > + break; > > + } > > + } > > + spin_unlock(&csdev->cfg_lock); > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(cscfg_csdev_enable_config); > > + > > +void cscfg_csdev_disable_config(struct coresight_device *csdev) > > +{ > > + struct cscfg_config_csdev *cfg; > > + > > + spin_lock(&csdev->cfg_lock); > > + list_for_each_entry(cfg, &csdev->syscfg_list, node) { > > + if (cfg->enabled) { > > + cscfg_prog_config(cfg, false); > > + cfg->enabled = false; > > + } > > + } > > + spin_unlock(&csdev->cfg_lock); > > +} > > +EXPORT_SYMBOL_GPL(cscfg_csdev_disable_config); > > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h > > index 8fd7ff991ced..a3d374ebb70f 100644 > > --- a/drivers/hwtracing/coresight/coresight-config.h > > +++ b/drivers/hwtracing/coresight/coresight-config.h > > @@ -287,4 +287,18 @@ struct cscfg_csdev_feat_ops { > > struct cscfg_feature_csdev *feat); > > }; > > > > +/* coresight config API functions */ > > + > > +/* helper functions for feature manipulation */ > > +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat); > > + > > +/* enable / disable features or configs on a device */ > > +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev); > > +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev); > > +void cscfg_csdev_reset_feats(struct coresight_device *csdev); > > +int cscfg_csdev_enable_config(struct coresight_device *csdev, > > + unsigned long cfg_id, > > + int preset); > > +void cscfg_csdev_disable_config(struct coresight_device *csdev); > > + > > #endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > > index 53bc1d551171..98ee78e3412a 100644 > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > > @@ -166,6 +166,7 @@ cscfg_alloc_csdev_feat(struct coresight_device *csdev, struct cscfg_feature_desc > > feat->csdev = csdev; > > for (i = 0; i < feat->nr_params; i++) > > feat->params[i].feat = feat; > > + cscfg_set_def_ops(feat); > > > > return feat; > > } > > @@ -195,6 +196,9 @@ static int cscfg_load_feat_csdev(struct coresight_device *csdev, > > list_add(&feat_csdev->node, &csdev->feat_list); > > spin_unlock(&csdev->cfg_lock); > > > > + /* force load of default parameter values into registers */ > > + feat_csdev->ops.reset(feat_csdev); > > + > > return 0; > > } > > > > -- > > 2.17.1 > > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 4ce854c434b1..daad9f103a78 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -4,7 +4,7 @@ # obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ - coresight-sysfs.o coresight-syscfg.o + coresight-sysfs.o coresight-syscfg.o coresight-config.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-config.c b/drivers/hwtracing/coresight/coresight-config.c new file mode 100644 index 000000000000..d911e0f083c1 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.c @@ -0,0 +1,362 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright(C) 2020 Linaro Limited. All rights reserved. + * Author: Mike Leach <mike.leach@linaro.org> + */ + +#include <linux/sysfs.h> +#include "coresight-config.h" +#include "coresight-priv.h" + +/* + * Write the value held in the register structure into the driver internal memory + * location. + */ +static void cscfg_set_reg(struct cscfg_reg_csdev *reg) +{ + u32 *p_val32 = (u32 *)reg->drv_store; + u32 tmp32; + + if (!(reg->value.flags & CS_CFG_REG_VAL_64BIT)) { + if (reg->value.flags & CS_CFG_REG_VAL_MASK) { + tmp32 = *p_val32; + tmp32 &= ~reg->value.mask32; + tmp32 |= reg->value.val32 & reg->value.mask32; + *p_val32 = tmp32; + } else + *p_val32 = reg->value.val32; + } else + *((u64 *)reg->drv_store) = reg->value.val64; +} + +/* + * Read the driver value into the reg if this is marked as one we want to save. + */ +static void cscfg_save_reg(struct cscfg_reg_csdev *reg) +{ + if (!(reg->value.flags & CS_CFG_REG_VAL_SAVE)) + return; + if (reg->value.flags & CS_CFG_REG_VAL_64BIT) + reg->value.val64 = *(u64 *)(reg->drv_store); + else + reg->value.val32 = *(u32 *)(reg->drv_store); +} + +static inline void cscfg_init_reg_val(struct cscfg_reg_csdev *reg, + const struct cscfg_regval *desc) +{ + reg->value.val64 = desc->val64; +} + +static inline void cscfg_init_reg_flags(struct cscfg_reg_csdev *reg, + const struct cscfg_regval *desc) +{ + reg->value.flags = desc->flags; +} + +static void cscfg_init_reg_param(struct cscfg_parameter_csdev *param, + struct cscfg_reg_csdev *reg) +{ + param->reg = reg; + param->val64 = reg->value.flags & CS_CFG_REG_VAL_64BIT; + + if (param->val64) + param->reg->value.val64 = param->current_value; + else + param->reg->value.val32 = (u32)param->current_value; +} + +/* default set - will set values without resource checking */ +static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat, const bool force_set) +{ + int i; + + spin_lock(feat->csdev_spinlock); + if (feat->enabled || force_set) { + for (i = 0; i < feat->nr_regs; i++) + cscfg_set_reg(&feat->regs[i]); + } + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, + feat->enabled || force_set ? "Set enabled" : "Skip disabled"); + spin_unlock(feat->csdev_spinlock); + return 0; +} + +static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat, const bool force_save) +{ + int i; + + spin_lock(feat->csdev_spinlock); + if (feat->enabled || force_save) { + for (i = 0; i < feat->nr_regs; i++) + cscfg_save_reg(&feat->regs[i]); + } + dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name, + feat->enabled || force_save ? "Save disabled" : "Skip disabled"); + spin_unlock(feat->csdev_spinlock); +} + +/* default reset - restore default values, disable feature */ +static void cscfg_reset_feat(struct cscfg_feature_csdev *feat) +{ + struct cscfg_reg_csdev *reg; + struct cscfg_regval *reg_desc; + struct cscfg_parameter_csdev *param; + int i; + + spin_lock(feat->csdev_spinlock); + feat->enabled = false; + + /* + * set the default values for all parameters and regs from the + * relevant static descriptors. + */ + for (i = 0; i < feat->nr_params; i++) + feat->params[i].current_value = feat->desc->params[i].value; + + for (i = 0; i < feat->nr_regs; i++) { + reg_desc = &feat->desc->regs[i]; + reg = &feat->regs[i]; + cscfg_init_reg_flags(reg, reg_desc); + + /* check if reg set from a parameter otherwise desc default */ + if (reg_desc->flags & CS_CFG_REG_VAL_PARAM) { + param = &feat->params[reg_desc->val32]; + cscfg_init_reg_param(param, reg); + } else + cscfg_init_reg_val(reg, reg_desc); + } + spin_unlock(feat->csdev_spinlock); +} + +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat) +{ + feat->ops.set_on_enable = cscfg_set_on_enable; + feat->ops.save_on_disable = cscfg_save_on_disable; + feat->ops.reset = cscfg_reset_feat; +} + +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev) +{ + struct cscfg_feature_csdev *feat; + int err = 0; + + spin_lock(&csdev->cfg_lock); + if (list_empty(&csdev->feat_list)) { + spin_unlock(&csdev->cfg_lock); + return 0; + } + + list_for_each_entry(feat, &csdev->feat_list, node) { + dev_dbg(&csdev->dev, "Found feature:%s", feat->desc->name); + + if (feat->ops.set_on_enable) { + err = feat->ops.set_on_enable(feat, false); + dev_dbg(&csdev->dev, "Feature %s: %s", + feat->desc->name, err ? "Set failed" : "OK"); + if (!err) + break; + } + } + spin_unlock(&csdev->cfg_lock); + return err; +} +EXPORT_SYMBOL_GPL(cscfg_csdev_set_enabled_feats); + +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev) +{ + struct cscfg_feature_csdev *feat; + + spin_lock(&csdev->cfg_lock); + if (list_empty(&csdev->feat_list)) { + spin_unlock(&csdev->cfg_lock); + return; + } + + list_for_each_entry(feat, &csdev->feat_list, node) { + if (feat->ops.save_on_disable) + feat->ops.save_on_disable(feat, false); + } + spin_unlock(&csdev->cfg_lock); +} +EXPORT_SYMBOL_GPL(cscfg_csdev_save_enabled_feats); + +void cscfg_csdev_reset_feats(struct coresight_device *csdev) +{ + struct cscfg_feature_csdev *feat; + + spin_lock(&csdev->cfg_lock); + if (list_empty(&csdev->feat_list)) { + spin_unlock(&csdev->cfg_lock); + return; + } + + list_for_each_entry(feat, &csdev->feat_list, node) { + if (feat->ops.reset) + feat->ops.reset(feat); + } + spin_unlock(&csdev->cfg_lock); +} +EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats); + +static int cscfg_update_presets(struct cscfg_config_csdev *cfg, int preset) +{ + int i, j, line_offset = 0, val_idx = 0, max_idx; + u64 val; + struct cscfg_feature_csdev *feat; + struct cscfg_parameter_csdev *param; + const char *name; + + if (preset > cfg->desc->nr_presets) + return -EINVAL; + /* + * Go through the array of features, assigning preset values to + * feature parameters in the order they appear. + * There should be precisely the same number of preset values as the + * sum of number of parameters over all the features - but we will + * ensure there is no overrun. + */ + line_offset = (preset-1) * cfg->desc->nr_total_params; + max_idx = cfg->desc->nr_total_params; + for (i = 0; i < cfg->nr_feat; i++) { + feat = cfg->feats[i]; + if (feat->nr_params) { + spin_lock(feat->csdev_spinlock); + for (j = 0; j < feat->nr_params; j++) { + param = &feat->params[j]; + name = feat->desc->params[j].name; + val = cfg->desc->presets[line_offset + val_idx++]; + if (param->val64) { + dev_dbg(&cfg->csdev->dev, + "set param %s (%lld)", name, val); + param->reg->value.val64 = val; + } else { + param->reg->value.val32 = (u32)val; + dev_dbg(&cfg->csdev->dev, + "set param %s (%d)", name, (u32)val); + } + if (val_idx >= max_idx) + break; + } + spin_unlock(feat->csdev_spinlock); + } + + /* don't overrun the preset array line */ + if (val_idx >= max_idx) + break; + } + return 0; +} + +/* + * if we are not using a preset, then need to update the feature params + * with current values. + */ +static int cscfg_update_curr_params(struct cscfg_config_csdev *cfg) +{ + int i, j; + struct cscfg_feature_csdev *feat; + struct cscfg_parameter_csdev *param; + const char *name; + u64 val; + + for (i = 0; i < cfg->nr_feat; i++) { + feat = cfg->feats[i]; + if (feat->nr_params) { + spin_lock(feat->csdev_spinlock); + for (j = 0; j < feat->nr_params; j++) { + param = &feat->params[j]; + name = feat->desc->params[j].name; + val = param->current_value; + if (param->val64) { + dev_dbg(&cfg->csdev->dev, + "set param %s (%lld)", name, val); + param->reg->value.val64 = val; + } else { + param->reg->value.val32 = (u32)val; + dev_dbg(&cfg->csdev->dev, + "set param %s (%d)", name, (u32)val); + } + } + spin_unlock(feat->csdev_spinlock); + } + } + return 0; +} + +static int cscfg_prog_config(struct cscfg_config_csdev *cfg, bool enable) +{ + int i, err = 0; + struct cscfg_feature_csdev *feat; + struct coresight_device *csdev; + + for (i = 0; i < cfg->nr_feat; i++) { + feat = cfg->feats[i]; + csdev = feat->csdev; + dev_dbg(&csdev->dev, "cfg %s; %s feature:%s", cfg->desc->name, + enable ? "enable" : "disable", feat->desc->name); + + if (enable) { + if (feat->ops.set_on_enable) + err = feat->ops.set_on_enable(feat, true); + } else { + if (feat->ops.save_on_disable) + feat->ops.save_on_disable(feat, true); + } + if (err) + break; + } + return err; +} + +/** + * Enable configuration for the device. + * Match the config id and optionally set preset values for parameters. + * + * @csdev: coresight device to set config on. + * @cfg_id: hash id of configuration. + * @preset: preset values to use - 0 for default. + */ +int cscfg_csdev_enable_config(struct coresight_device *csdev, + unsigned long cfg_id, + int preset) +{ + struct cscfg_config_csdev *cfg; + int err = 0; + + dev_dbg(&csdev->dev, "Check for config %lx, preset %d", cfg_id, preset); + + spin_lock(&csdev->cfg_lock); + list_for_each_entry(cfg, &csdev->syscfg_list, node) { + dev_dbg(&csdev->dev, "checking %s", cfg->desc->name); + if (cfg->id_hash == cfg_id) { + if (preset) + err = cscfg_update_presets(cfg, preset); + else + err = cscfg_update_curr_params(cfg); + if (!err) + err = cscfg_prog_config(cfg, true); + if (!err) + cfg->enabled = true; + break; + } + } + spin_unlock(&csdev->cfg_lock); + return err; +} +EXPORT_SYMBOL_GPL(cscfg_csdev_enable_config); + +void cscfg_csdev_disable_config(struct coresight_device *csdev) +{ + struct cscfg_config_csdev *cfg; + + spin_lock(&csdev->cfg_lock); + list_for_each_entry(cfg, &csdev->syscfg_list, node) { + if (cfg->enabled) { + cscfg_prog_config(cfg, false); + cfg->enabled = false; + } + } + spin_unlock(&csdev->cfg_lock); +} +EXPORT_SYMBOL_GPL(cscfg_csdev_disable_config); diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 8fd7ff991ced..a3d374ebb70f 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -287,4 +287,18 @@ struct cscfg_csdev_feat_ops { struct cscfg_feature_csdev *feat); }; +/* coresight config API functions */ + +/* helper functions for feature manipulation */ +void cscfg_set_def_ops(struct cscfg_feature_csdev *feat); + +/* enable / disable features or configs on a device */ +int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev); +void cscfg_csdev_save_enabled_feats(struct coresight_device *csdev); +void cscfg_csdev_reset_feats(struct coresight_device *csdev); +int cscfg_csdev_enable_config(struct coresight_device *csdev, + unsigned long cfg_id, + int preset); +void cscfg_csdev_disable_config(struct coresight_device *csdev); + #endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 53bc1d551171..98ee78e3412a 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -166,6 +166,7 @@ cscfg_alloc_csdev_feat(struct coresight_device *csdev, struct cscfg_feature_desc feat->csdev = csdev; for (i = 0; i < feat->nr_params; i++) feat->params[i].feat = feat; + cscfg_set_def_ops(feat); return feat; } @@ -195,6 +196,9 @@ static int cscfg_load_feat_csdev(struct coresight_device *csdev, list_add(&feat_csdev->node, &csdev->feat_list); spin_unlock(&csdev->cfg_lock); + /* force load of default parameter values into registers */ + feat_csdev->ops.reset(feat_csdev); + return 0; }
Adds a set of generic support functions that allow devices to set and save features values on the device, and enable and disable configurations. Additional functions for other common operations including feature reset. Signed-off-by: Mike Leach <mike.leach@linaro.org> --- drivers/hwtracing/coresight/Makefile | 2 +- .../hwtracing/coresight/coresight-config.c | 362 ++++++++++++++++++ .../hwtracing/coresight/coresight-config.h | 14 + .../hwtracing/coresight/coresight-syscfg.c | 4 + 4 files changed, 381 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-config.c