On Tue, Oct 06, 2020 at 12:18:51PM +0100, Mike Leach wrote:
Hi Mathieu,
On Thu, 1 Oct 2020 at 22:20, Mathieu Poirier mathieu.poirier@linaro.org wrote:
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?
This is linked to the number of bits (4) used to select presets in the perf config2 value, introduced later in the perf updates. We can certainly increase the number of bits ./ max entries - but this seems like a reasonable starting point.
Oh boy - the relation with the 4 bits in config2 was completely lost on me. Adding a comment to clearly identify the link between the two would go a long way.
That being said I remember frowning a little when I looked at how config2 was used and something I will pay close attention to in this second pass.
+/**
- 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?
Correct. Values in the description of the feature form the defaults for programming the device. These can be absolute values - which are used directly, resource values - where the loading process will select an appropriate resource (e.g. etm counter), or parameter value, which has a default value that can be updated at runtime via sysfs / configfs.
+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?
The structures are split into 'descriptors' - which describe a feature / configuration and 'instances' which represent the feature loaded into a specific device. So in a system with 4 ETMv4 , a single feature descriptor for ETMv4, will result in 4 instances loaded into the respective ETM devices. The reg_val appears in both.
That said, for simplification purposes, we could move the flags into the value and drop this structure at the cost of a very marginal memory increase.
Thanks
+/**
- 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.
cs_cfg_feature exists as the structure used for the instance of a feature loaded into a device, introduced in a later patch.
My goal is to find a different label than "uses". It took me a while wrap my head around it and even after that I kept tripping on it.
Maybe keep it as it is for now and we can revisit as this set matures.
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.
Will do.
/* 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.
will do.
int nr_params;Shouldn't this be defined in cs_cfg_feature_desc?
yes - that would be better.
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?
The cs_cfg_parameter_desc represent the default parameter values for the feature - which can be loaded / used independently of a configuration that provides presets.
+};
+#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().
At present, configurations & features can only be loaded via the built-in block of code. This really shouldn't have errors - but we should probably have a least a message here.. The error handling is broken as has been pointed out in Yabin's mail.
Ok
Once dynamic loading is permitted - either by loadable module or though the configfs - the handing of unload will be added. For a given set, failing to load a configuration does not necessarily invalidate the previous features, which can be used directly or in other configurations. At present the memory associated with loading configurations and features into the device is allocate using the devm_ allocators associated with the virtual syscfg device - and thus is correctly cleaned up.
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.
agreed.
+/**
- 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.
agreed.
/* 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
Thanks for the review
Regards
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK