Hi Mathieu, Thanks for looking at this.
However, per your comment in the v1 set, for the next set I have moved management of features out of individual coresight device sysfs, and into a custom configfs directory. This actually makes things easter for the user - they can change a single parameter value and have it change on all device instances of the feature.
Regards
Mike
On Fri, 9 Oct 2020 at 22:34, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, Aug 27, 2020 at 03:34:07PM +0100, 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 | 565 ++++++++++++++++++ .../hwtracing/coresight/coresight-config.h | 16 + 3 files changed, 582 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 3605dd770664..177bc6338312 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..96f8df59df58 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.c @@ -0,0 +1,565 @@ +// 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"
+static void coresight_cfg_init_reg_param(struct cs_cfg_parameter *param);
+/*
- Handlers for creating and manipulating the feature representation
- in sysfs.
- */
+/* base feature attribute info */ +struct feat_attr_info {
const char *name;
ssize_t (*show)(struct device *dev, struct device_attribute *attr,
char *buf);
ssize_t (*store)(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
+};
+static ssize_t cs_cfg_enable_show(struct device *dev,
struct device_attribute *attr,
char *buf)
There is no need to have "char *buf" on a line by itself. Same for cs_cfg_param_show().
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
int val;
spin_lock(feat->dev_spinlock);
I would rename cs_cfg_featre::dev_spinlock to cs_cfg_feature::csdev_spinlock to remove any ambiguity.
val = feat->enabled;
spin_unlock(feat->dev_spinlock);
return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+static ssize_t cs_cfg_enable_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
int val;
spin_lock(feat->dev_spinlock);
if (!kstrtoint(buf, 0, &val))
Please call kstrtoint() outside of the lock.
feat->enabled = val;
spin_unlock(feat->dev_spinlock);
return size;
+}
+static ssize_t cs_cfg_desc_show(struct device *dev,
struct device_attribute *attr,
char *buf)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
return scnprintf(buf, PAGE_SIZE, "%s\n", feat->desc->brief);
+}
+struct feat_attr_info feature_std_attr[] = {
{
.name = "enable",
.show = cs_cfg_enable_show,
.store = cs_cfg_enable_store,
},
{
.name = "description",
.show = cs_cfg_desc_show,
},
{ 0 },
+};
+static ssize_t cs_cfg_param_show(struct device *dev,
struct device_attribute *attr,
char *buf)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_parameter *param = (struct cs_cfg_parameter *)eattr->var;
u64 val;
spin_lock(param->feat->dev_spinlock);
val = param->current_value;
spin_unlock(param->feat->dev_spinlock);
return scnprintf(buf, PAGE_SIZE, "%lld\n", val);
+}
+static ssize_t cs_cfg_param_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_parameter *param = (struct cs_cfg_parameter *)eattr->var;
u64 val;
spin_lock(param->feat->dev_spinlock);
if (!kstrtoull(buf, 0, &val)) {
Same here.
param->current_value = val;
coresight_cfg_init_reg_param(param);
}
spin_unlock(param->feat->dev_spinlock);
return size;
+}
+static struct attribute * +cs_cfg_sysfs_create_feat_attr(struct device *dev, struct cs_cfg_feature *feat,
struct feat_attr_info *info)
+{
struct dev_ext_attribute *eattr;
eattr = devm_kzalloc(dev, sizeof(struct dev_ext_attribute),
GFP_KERNEL);
if (!eattr)
return NULL;
For concistency I would have expected a space here, as it is in cs_cfg_sysfs_create_param_attr().
eattr->attr.attr.name = devm_kstrdup(dev, info->name, GFP_KERNEL);
eattr->attr.attr.mode = info->store ? 0644 : 0444;
eattr->attr.show = info->show;
eattr->attr.store = info->store;
eattr->var = feat;
return &eattr->attr.attr;
+}
+static struct attribute * +cs_cfg_sysfs_create_param_attr(struct device *dev, const char *name,
struct cs_cfg_parameter *param)
+{
struct dev_ext_attribute *eattr;
eattr = devm_kzalloc(dev, sizeof(struct dev_ext_attribute),
GFP_KERNEL);
if (!eattr)
return NULL;
eattr->attr.attr.name = devm_kstrdup(dev, name, GFP_KERNEL);
eattr->attr.attr.mode = 0644;
eattr->attr.show = cs_cfg_param_show;
eattr->attr.store = cs_cfg_param_store;
eattr->var = param;
return &eattr->attr.attr;
+}
+int coresight_cfg_sysfs_add_grp(struct cs_cfg_feature *feat) +{
int p, attr_idx = 0;
struct attribute **attrs;
const char *name;
struct feat_attr_info *info;
struct device *dev = feat->csdev->dev.parent;
/* create sysfs group in csdev */
feat->param_group = devm_kzalloc(dev, sizeof(struct attribute_group),
GFP_KERNEL);
if (!feat->param_group)
return -ENOMEM;
feat->param_group->name = devm_kasprintf(dev, GFP_KERNEL, "%s.feat",
feat->desc->name);
if (!feat->param_group->name)
return -ENOMEM;
/* attributes - nr_params + enable + desc */
attrs = devm_kcalloc(dev, feat->nr_params + 2,
sizeof(struct attribute *), GFP_KERNEL);
@attrs needs to be NULL terminated and I'm surprised it hasn't blown up in your face yet (or I've missed the NULL terminated slot).
if (!attrs)
return -ENOMEM;
/* create base attributes in group */
while (feature_std_attr[attr_idx].name) {
info = &feature_std_attr[attr_idx];
attrs[attr_idx] = cs_cfg_sysfs_create_feat_attr(dev, feat,
info);
if (!attrs[attr_idx])
return -ENOMEM;
attr_idx++;
}
/* create params in group */
for (p = 0; p < feat->nr_params; p++) {
name = feat->desc->params[p].name;
attrs[attr_idx] =
cs_cfg_sysfs_create_param_attr(dev, name,
&feat->params[p]);
attrs[attr_idx] = cs_cfg_sysfs_create_param_attr(dev, name, &feat->params[p]);
if (!attrs[attr_idx])
return -ENOMEM;
attr_idx++;
}
feat->param_group->attrs = attrs;
return sysfs_create_group(&feat->csdev->dev.kobj, feat->param_group);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_sysfs_add_grp);
+/*
- standard routines to set/save/reset enabled features, and iterate
- groups of features on a device.
- */
+void coresight_cfg_list_add_feat(struct cs_cfg_feature *feat) +{
spin_lock(&feat->csdev->cfg_lock);
list_add(&feat->node, &feat->csdev->feat_list);
spin_unlock(&feat->csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_list_add_feat);
+static void coresight_cfg_set_reg(struct cs_cfg_reg *reg, u32 flags) +{
u32 *p_val32 = (u32 *)reg->drv_store;
u32 tmp32;
if (!(flags & CS_CFG_REG_VAL_64BIT)) {
if (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;
+}
+static void coresight_cfg_save_reg(struct cs_cfg_reg *reg, u32 flags) +{
if (flags & CS_CFG_REG_VAL_64BIT)
reg->value.val64 = *(u64 *)(reg->drv_store);
else
reg->value.val32 = *(u32 *)(reg->drv_store);
+}
+static void coresight_cfg_init_reg(struct cs_cfg_reg *reg,
const struct cs_cfg_reg_desc *desc)
Indentation problem.
It is hard to look a the rest of this patch without more context. As such I will come back to it as I look a the other patches in this series.
Thanks, Mathieu
+{
reg->value.val64 = desc->value.val64;
+}
+static void coresight_cfg_init_reg_param(struct cs_cfg_parameter *param) +{
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 cs_cfg_set_on_enable(struct cs_cfg_feature *feat, const bool force_set) +{
int i;
u32 flags;
spin_lock(feat->dev_spinlock);
if (feat->enabled || force_set) {
for (i = 0; i < feat->nr_regs; i++) {
flags = feat->desc->regs[i].flags;
coresight_cfg_set_reg(&feat->regs[i], flags);
}
}
dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name,
feat->enabled || force_set ? "Set enabled" : "Skip disabled");
spin_unlock(feat->dev_spinlock);
return 0;
+}
+static void cs_cfg_save_on_disable(struct cs_cfg_feature *feat, const bool force_save) +{
int i;
u32 flags;
spin_lock(feat->dev_spinlock);
if (feat->enabled || force_save) {
for (i = 0; i < feat->nr_regs; i++) {
flags = feat->desc->regs[i].flags;
if (flags & CS_CFG_REG_VAL_SAVE)
coresight_cfg_save_reg(&feat->regs[i], flags);
}
}
dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name,
feat->enabled || force_save ? "Save disabled" : "Skip disabled");
spin_unlock(feat->dev_spinlock);
+}
+/* default reset - restore default values, disable feature */ +static void cs_cfg_reset_feat(struct cs_cfg_feature *feat) +{
const struct cs_cfg_feature_desc *feat_desc = feat->desc;
struct cs_cfg_reg_desc *reg_desc;
struct cs_cfg_parameter *param;
struct cs_cfg_reg *reg;
int i;
u32 flags;
spin_lock(feat->dev_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];
flags = reg_desc->flags;
reg = &feat->regs[i];
/* check if reg set from a parameter otherwise desc default */
if (flags & CS_CFG_REG_VAL_PARAM) {
param = &feat->params[reg_desc->value.val32];
param->reg = reg;
param->val64 = flags & CS_CFG_REG_VAL_64BIT;
coresight_cfg_init_reg_param(param);
} else
coresight_cfg_init_reg(reg, reg_desc);
}
spin_unlock(feat->dev_spinlock);
+}
+void coresight_cfg_set_def_ops(struct cs_cfg_feature *feat) +{
feat->ops.set_on_enable = cs_cfg_set_on_enable;
feat->ops.save_on_disable = cs_cfg_save_on_disable;
feat->ops.reset = cs_cfg_reset_feat;
+} +EXPORT_SYMBOL_GPL(coresight_cfg_set_def_ops);
+int coresight_cfg_set_enabled_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *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(coresight_cfg_set_enabled_feats);
+void coresight_cfg_save_enabled_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *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(coresight_cfg_save_enabled_feats);
+void coresight_cfg_reset_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *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(coresight_cfg_reset_feats);
+static int coresight_cfg_update_presets(struct cs_cfg_config_dev *cfg, int preset) +{
int i, j, line_offset = 0, val_idx = 0, max_idx;
u64 val;
struct cs_cfg_feature *feat;
struct cs_cfg_parameter *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->dev_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->dev_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 coresight_cfg_update_curr_params(struct cs_cfg_config_dev *cfg) +{
int i, j;
struct cs_cfg_feature *feat;
struct cs_cfg_parameter *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->dev_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->dev_spinlock);
}
}
return 0;
+}
+static int coresight_cfg_prog_config(struct cs_cfg_config_dev *cfg, bool enable) +{
int i, err = 0;
struct cs_cfg_feature *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 coresight_cfg_enable_dev_config(struct coresight_device *csdev,
unsigned long cfg_id,
int preset)
+{
struct cs_cfg_config_dev *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 = coresight_cfg_update_presets(cfg, preset);
else
err = coresight_cfg_update_curr_params(cfg);
if (!err)
err = coresight_cfg_prog_config(cfg, true);
if (!err)
cfg->enabled = true;
break;
}
}
spin_unlock(&csdev->cfg_lock);
return err;
+} +EXPORT_SYMBOL_GPL(coresight_cfg_enable_dev_config);
+void coresight_cfg_disable_dev_config(struct coresight_device *csdev) +{
struct cs_cfg_config_dev *cfg;
spin_lock(&csdev->cfg_lock);
list_for_each_entry(cfg, &csdev->syscfg_list, node) {
if (cfg->enabled) {
coresight_cfg_prog_config(cfg, false);
cfg->enabled = false;
}
}
spin_unlock(&csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_disable_dev_config); diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 949a078c4692..afecab88451a 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -270,4 +270,20 @@ struct cs_cfg_dev_feat_ops { const struct cs_cfg_feature_desc *feat_desc); };
+/* coresight config API functions */
+/* helper functions for feature manipulation */ +int coresight_cfg_sysfs_add_grp(struct cs_cfg_feature *feat); +void coresight_cfg_set_def_ops(struct cs_cfg_feature *feat); +void coresight_cfg_list_add_feat(struct cs_cfg_feature *feat);
+/* enable / disable features or configs on a device */ +int coresight_cfg_set_enabled_feats(struct coresight_device *csdev); +void coresight_cfg_save_enabled_feats(struct coresight_device *csdev); +void coresight_cfg_reset_feats(struct coresight_device *csdev); +int coresight_cfg_enable_dev_config(struct coresight_device *csdev,
unsigned long cfg_id,
int preset);
+void coresight_cfg_disable_dev_config(struct coresight_device *csdev);
#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */
2.17.1