Re: [RFC PATCH v2 2/8] coresight: syscfg: Add loading of features and configurations

On Thu, Aug 27, 2020 at 03:34:05PM +0100, Mike Leach wrote:

Add API call to load features and configurations into the system lists.

Defines generic structures and flags needed to describe features and configurations.

Signed-off-by: Mike Leach mike.leach@linaro.org

.../hwtracing/coresight/coresight-config.h | 154 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.c | 116 +++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 26 +++ 3 files changed, 296 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-config.h

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h new file mode 100644 index 000000000000..413509f7d817 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -0,0 +1,154 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_CORESIGHT_CONFIG_H +#define _CORESIGHT_CORESIGHT_CONFIG_H

+#include <linux/coresight.h> +#include <linux/types.h>

+/* CoreSight Configuration - complex component and system config handling */

+/* generic register flags */ +#define CS_CFG_REG_STD 0x80000000 /* reg is standard reg */ +#define CS_CFG_REG_RESOURCE 0x40000000 /* reg is a resource */ +#define CS_CFG_REG_VAL_PARAM 0x08000000 /* reg value uses param */ +#define CS_CFG_REG_VAL_MASK 0x04000000 /* reg value bit masked */ +#define CS_CFG_REG_VAL_64BIT 0x02000000 /* reg value 64 bit */ +#define CS_CFG_REG_VAL_SAVE 0x01000000 /* reg value save on disable */ +#define CS_CFG_REG_ID_MASK 0x00000FFF /* reg offset / id in device */ +#define CS_CFG_REG_DEV_MASK 0x00FFF000 /* device specific flags */

+/*

• • flags defining what device class a feature will match to when processing a
• • system configuration - used by config data and devices.
• */

+#define CS_CFG_MATCH_CLASS_SRC_ALL 0x0001 /* match any source */ +#define CS_CFG_MATCH_CLASS_SRC_ETM4 0x0002 /* match any ETMv4 device */ +#define CS_CFG_MATCH_CLASS_CTI_ALL 0x0004 /* match any CTI device */ +#define CS_CFG_MATCH_CLASS_CTI_CPU 0x0008 +#define CS_CFG_MATCH_CLASS_CTI_SYS 0x0010

+/* flags defining device instance matching - used in config data. */ +#define CS_CFG_MATCH_INST_ANY 0x80000000 /* any instance of a class */ +#define CS_CFG_MATCH_INST_AMBA_DEVICE 0x40000000 /* specific amba device */

+/* limit number of presets in a configuration */ +#define CS_CFG_CONFIG_PRESET_MAX 15

Any reason to set the maximum number of presets at 15? Could it be a list to be more flexible?

+/**

• • Parameter descriptor for a device feature.
• • @name: Name of parameter.
• • @value: Initial or default value.
• */

+struct cs_cfg_parameter_desc {

• const char *name;
• u64 value;

+};

+/**

• • Representation of register value.
• • Supports full 64 bit register value, or 32 bit value with optional mask
• • value.
• */

+union cs_cfg_regval {

• u64 val64;
• struct {

• u32 val32;
  

• u32 mask32;
  

• };

+};

+/**

• • Register descriptor for a device feature.
• • Flags define 32bit, 32bit+mask, 64bit.
• • @flags: flags defining the handling of the register.
• • @value: Initial value & optional mask.
• */

The initial values of registers could be sufficient to work with a feature - is my understanding correct?

+struct cs_cfg_reg_desc {

• u32 flags;
• union cs_cfg_regval value;

+};

Is this structure absolutely required? Can @flag be added to cs_cfg_regval to avoid the declaration of a new structure?

+/**

• • Device feature descriptor - combination of registers and parameters to
• • program a device to implement a specific complex function.
• • @name: feature name.
• • @brief: brief description of the feature.
• • @match_flags: matching information if loading into a device
• • @params: list of parameters used - 0 terminated.
• • @reg: list of registers used.
• */

+struct cs_cfg_feature_desc {

I suggest calling this cs_cfg_feature. That way we can rename cs_cfg_uses_feat_desc to cs_cfg_feature_desc and drop the "uses" part.

• const char *name;
• const char *brief;
• u32 match_flags;
• struct cs_cfg_parameter_desc *params;
• struct cs_cfg_reg_desc *regs;

+};

+/**

• • Device / feature matching information.
• • Used by loading configurations to define which class or specific devices
• • they want to match used features to, and registered devices to specify which
• • matching class and information they support.
• • The load process uses these matching pairs to load feature instances into
• • matching devices.
• */

+struct cs_cfg_match_info {

• u32 flags; /* match flags */

Please use "flags" or "match_flags" but not both.

• /* unique device name (e.g. amba bus name) for specific device match */
• const char *dev_name;

+};

+/**

• • Uses feature description.
• • @name: name of feature to use.
• • @nr_params: number of parameters the feature has.
• • @match: match info for the feature in use - may be all devices of a

• • class or a specific device in that class.
    

• */

+struct cs_cfg_uses_feat_desc {

• const char *name;

If I remember correctly @name needs to match with cs_cfg_feature_desc::name. If that is the case please add a comment to explicitly link them. That will make it easier for people to make the connection.

• int nr_params;

Shouldn't this be defined in cs_cfg_feature_desc?

• struct cs_cfg_match_info match;

+};

+/**

• • Configuration descriptor - describes selectable system configuration.
• • A configuration describes device features in use, and may provide preset
• • values for the parameters in those features.
• • A single set of presets is the sum of the parameters declared by
• • all the features in use - this value is @nr_total_params.
• • @name: name of the configuration - used for selection.
• • @brief: description of the purpose of the configuration.
• • @nr_uses: Number of features used in this configuration.
• • @used: feature use descriptors.
• • @nr_presets: Number of sets of presets supplied by this configuration.
• • @nr_total_params: Sum of all parameters declared by used features
• • @presets: Array of preset values.
• */

+struct cs_cfg_config_desc {

• const char *name;
• const char *brief;
• int nr_uses;

s/nr_uses/nr_feature_desc

• struct cs_cfg_uses_feat_desc *uses;

s/cs_cfg_uses_feat_desc *uses/cs_cfg_feature_desc *feature_desc

• int nr_presets;
• int nr_total_params;
• const u64 *presets; /* nr_presets * nr_total_params */

What is the difference between presets and the values held by cs_cfg_parameter_desc? Can we get rid of the latter?

+};

+#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index badef8cfc7f8..d49bcace76c8 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -6,6 +6,7 @@ #include <linux/platform_device.h> +#include "coresight-config.h" #include "coresight-syscfg.h" @@ -28,6 +29,121 @@ static struct device_type cscfg_dev_type = { #define to_device_cscfg() (&cscfg_dev->csdev.dev) #define to_coresight_device_cscfg() (&cscfg_dev->csdev) +/* load features and configuations into the lists */

+/* check feature list for a named feature - call with mutex locked. */ +static bool cscfg_match_list_feat(const char *name) +{

• struct cscfg_featlist_item *curr_item;
• list_for_each_entry(curr_item, &cscfg_dev->feat_list, item) {

• if (strcmp(curr_item->desc->name, name) == 0)
  

• 	return true;
  

• }
• return false;

+}

+/* check all feat needed for cfg are in the list - call with mutex locked. */ +static int cscfg_check_feat_for_cfg(struct cs_cfg_config_desc *cfg_desc) +{

• int i;
• for (i = 0; i < cfg_desc->nr_uses; i++)

• if (!cscfg_match_list_feat(cfg_desc->uses[i].name))
  

• 	return -EINVAL;
  

• return 0;

+}

+/*

• • load feature - add to feature list.
• */

+static int cscfg_load_feat(struct cs_cfg_feature_desc *feat_desc) +{

• struct cscfg_featlist_item *feat_item;
• if (!cscfg_dev)

• return -ENODEV;
  

• /* create a list item and add to the list */
• feat_item = devm_kzalloc(to_device_cscfg(),

• 		 sizeof(struct cscfg_featlist_item),
  

• 		 GFP_KERNEL);
  

• if (!feat_item)

• return -ENOMEM;
  

• feat_item->desc = feat_desc;
• list_add(&feat_item->item, &cscfg_dev->feat_list);
• return 0;

+}

+/*

• • load config into the system - validate used features exist then add to
• • config list.
• */

+static int cscfg_load_config(struct cs_cfg_config_desc *cfg_desc) +{

• int err = -ENODEV;
• struct cs_cfg_config *cfg;
• if (!cscfg_dev)

• return -ENODEV;
  

• /* validate features are present */
• err = cscfg_check_feat_for_cfg(cfg_desc);
• if (err)

• return err;
  

• /* add to list */
• cfg = devm_kzalloc(to_device_cscfg(),

• 	   sizeof(struct cs_cfg_config), GFP_KERNEL);
  

• if (!cfg)

• return -ENOMEM;
  

• cfg->desc = cfg_desc;
• list_add(&cfg->item, &cscfg_dev->config_list);
• return 0;

+}

+/*

• • External API function to load feature and config sets.
• • Take a 0 terminated array of feature descriptors and/or configuration
• • descriptors and load into the system.
• • Features are loaded first to ensure configuration dependencies can be met.
• */

+int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs,

• 	   struct cs_cfg_feature_desc **feat_descs)
  

+{

• int err, i = 0;
• mutex_lock(&cs_cfg_mutex);
• /* load features first */
• if (feat_descs) {

• while (feat_descs[i]) {
  

• 	err = cscfg_load_feat(feat_descs[i]);
  

• 	if (err)
  

• 		return err;
  

• 	i++;
  

• }
  

• }
• /* next any configurations to check feature dependencies */
• i = 0;
• if (cfg_descs) {

• while (cfg_descs[i]) {
  

• 	err = cscfg_load_config(cfg_descs[i]);
  

• 	if (err)
  

• 		return err;
  

• 	i++;
  

• }
  

• }

Features and configurations aren't unloaded when an error occurs. That is probably something that could go in csfg_device_release().

• mutex_unlock(&cs_cfg_mutex);
• return err;

+} +EXPORT_SYMBOL_GPL(cscfg_load_config_sets); /* cs_syscfg device instance handling */ static void cscfg_device_release(struct device *dev) diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h index 20ccca5e4151..b3d41096382d 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.h +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -30,10 +30,36 @@ struct cs_cfg_driver { struct device_driver drv; }; +/**

• • List entry for feature loaded into the system feature list.
• • @desc: Descriptor for the feature.
• • @item: List entry
• */

+struct cscfg_featlist_item {

• const struct cs_cfg_feature_desc *desc;
• struct list_head item;

+};

I would add the list_head to structure cs_cfg_feature_desc and get rid of cscfg_featlist_item.

+/**

• • List entry for configuration added to system config list.
• • @desc: Descriptor for the configuration.
• • @item: List entry.
• */

+struct cs_cfg_config {

• const struct cs_cfg_config_desc *desc;
• struct list_head item;

+};

Same as above.

/* internal core operations for cscfg */ int cscfg_create_device(void); void cscfg_clear_device(void); int __init cscfg_driver_init(void); void cscfg_driver_exit(void); +/* syscfg external API */ +int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs,

• 	   struct cs_cfg_feature_desc **feat_descs);
  

#endif /* CORESIGHT_SYSCFG_H */

2.17.1