Re: [RFC PATCH v3 3/9] coresight: config: Add configuration and feature generic functions

[Mike Leach <mike.leach@linaro.org>]

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

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