Re: [RFC PATCH v2 1/8] coresight: syscfg: Initial coresight system configuration

On Tue, Oct 06, 2020 at 11:40:46AM +0100, Mike Leach wrote:

Hi Mathieu,

On Thu, 1 Oct 2020 at 21:07, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

Good afternoon Mike,

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

...

Creates a virtual coresight device on the bus to handle system configuration of the coresight infrastructure.

The device will manage system wide configuration, and allow complex programmed features to be added to individual device instances, and provide for system wide configuration selection on trace capture operations.

This patch updates the coresight core initialisation to create the single instance of the cs_syscfg coresight system device, on the coresight bus.

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

drivers/hwtracing/coresight/Makefile | 2 +- drivers/hwtracing/coresight/coresight-core.c | 29 +++- .../hwtracing/coresight/coresight-etm-perf.c | 2 +- .../hwtracing/coresight/coresight-etm-perf.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 136 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 39 +++++ include/linux/coresight.h | 1 + 7 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.c create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 1b35b55bd420..3605dd770664 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-sysfs.o coresight-syscfg.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-core.c b/drivers/hwtracing/coresight/coresight-core.c index bf6edf468963..91f1fbd7d51d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -21,6 +21,7 @@

#include "coresight-etm-perf.h" #include "coresight-priv.h" +#include "coresight-syscfg.h"

static DEFINE_MUTEX(coresight_mutex);

@@ -1669,8 +1670,17 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, } EXPORT_SYMBOL_GPL(coresight_alloc_device_name);

+/* match for virtual csdevs that have no underlying amba device */ +static int coresight_bus_match(struct device *dev, struct device_driver *drv) +{

• struct coresight_device *csdev = to_coresight_device(dev);
  

• return csdev->type == CORESIGHT_DEV_TYPE_SYSCFG;
  

+}

struct bus_type coresight_bustype = { .name = "coresight",

• .match  = coresight_bus_match,
  

};

static int __init coresight_init(void) @@ -1683,13 +1693,29 @@ static int __init coresight_init(void)

  ret = etm_perf_init();
  if (ret)

•         bus_unregister(&coresight_bustype);
  

•         goto exit_bus_unregister;
  

• /* initialise the coresight syscfg driver and device */
  

• ret = cscfg_driver_init();
  

• if (ret)
  

•         goto exit_perf_clear;
  

• ret = cscfg_create_device();
  

• if (!ret)
  

•         return 0;
  
  

• cscfg_driver_exit();
  

+exit_perf_clear:

• etm_perf_exit();
  

+exit_bus_unregister:

• bus_unregister(&coresight_bustype);
  return ret;
  

}

static void __exit coresight_exit(void) {

• cscfg_clear_device();
  

• cscfg_driver_exit();
  etm_perf_exit();
  bus_unregister(&coresight_bustype);
  

} @@ -1700,3 +1726,4 @@ module_exit(coresight_exit); MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org"); MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); MODULE_DESCRIPTION("Arm CoreSight tracer driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 668b3ff11576..2d619b764738 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -619,7 +619,7 @@ int __init etm_perf_init(void) return ret; }

-void __exit etm_perf_exit(void) +void etm_perf_exit(void) { perf_pmu_unregister(&etm_pmu); } diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 3e4f2ad5e193..29d90dfeba31 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -83,6 +83,6 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #endif /* CONFIG_CORESIGHT */

int __init etm_perf_init(void); -void __exit etm_perf_exit(void); +void etm_perf_exit(void);

#endif diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c new file mode 100644 index 000000000000..badef8cfc7f8 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

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

+#include <linux/platform_device.h>

+#include "coresight-syscfg.h"

+/*

• • cs_syscfg device is a virtual device managing the overall cs system.
• • It allows the loading of configurations and features, and loads these into
• • coresight devices as appropriate.
• */

+static DEFINE_MUTEX(cs_cfg_mutex);

+/* only one of these */ +static struct cs_cfg_device *cscfg_dev;

+static struct device_type cscfg_dev_type = {

• .name = "cs_syscfg",
  

+};

+#define to_device_cscfg() (&cscfg_dev->csdev.dev) +#define to_coresight_device_cscfg() (&cscfg_dev->csdev)

+/* cs_syscfg device instance handling */ +static void cscfg_device_release(struct device *dev) +{

• kfree(cscfg_dev->csdev.pdata);
  

• kfree(cscfg_dev);
  

• cscfg_dev = NULL;
  

+}

+int cscfg_create_device(void) +{

• struct device *dev;
  

• struct coresight_device *csdev;
  

• struct coresight_platform_data *pdata = NULL;
  

• int err = -ENOMEM;
  

• mutex_lock(&cs_cfg_mutex);
  

• if (cscfg_dev) {
  

•         err = -EINVAL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• cscfg_dev = kzalloc(sizeof(*cscfg_dev), GFP_KERNEL);
  

• if (!cscfg_dev)
  

•         goto create_dev_exit_unlock;
  

• pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
  

• if (!pdata) {
  

•         kfree(cscfg_dev);
  

•         cscfg_dev = NULL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• /* setup the device */
  

• dev = to_device_cscfg();
  

• dev_set_name(dev, "cs_syscfg");
  

• dev->bus = &coresight_bustype;
  

• dev->parent = &platform_bus;
  

• dev->type = &cscfg_dev_type;
  

As far as I can tell setting the device type doesn't do anything.

It provides name for the entry in /sys/devices/... - ie.

/sys/devices/platform/cs_syscfg

The device name is set in dev_set_name(), which is sufficient for proper labelling in sysfs.

...

...

• /* setup a coresight device */
  

• csdev = to_coresight_device_cscfg();
  

• csdev->type = CORESIGHT_DEV_TYPE_SYSCFG;
  

• csdev->pdata = pdata;
  

• csdev->orphan = false;
  

• csdev->dev.release = cscfg_device_release;
  

I also fail to understand the requirement to use a coresight_device and to add it to the coresigh bus. To me it seems odd to have it there since it has no interaction with any of the other CS devices.

Not at this early stage - this patch just introduces the device, without much functionality at present. But later on it becomes capable of loading features and configurations and enabling them across relevant devices on the bus.

The bus_type is very rich in feature and should provide everything you need. Using a csdev should be the very last option.

...

...

• err = device_register(dev);
  

+create_dev_exit_unlock:

• mutex_unlock(&cs_cfg_mutex);
  

• return err;
  

+}

+void cscfg_clear_device(void) +{

• mutex_lock(&cs_cfg_mutex);
  

• device_unregister(to_device_cscfg());
  

• mutex_unlock(&cs_cfg_mutex);
  

+}

+/* Finish allocating resources and load built-in configurations and features. */ +static int cscfg_probe(struct device *dev) +{

• int ret = 0;
  

• /* there can be only one... */
  

• if (dev != to_device_cscfg())
  

•         return -EINVAL;
  

• /*
  

•  * No actual hardware to handle, just allocate resources.
  

•  * Probe is triggered when the core calls cscfg_create_device() to
  

•  * create the device, so the mutex is already held.
  

•  */
  

• INIT_LIST_HEAD(&cscfg_dev->dev_list);
  

• INIT_LIST_HEAD(&cscfg_dev->feat_list);
  

• INIT_LIST_HEAD(&cscfg_dev->config_list);
  

• dev_info(dev, "CSCFG initialised");
  

• return ret;
  

+}

+/* driver initialisation */ +static struct cs_cfg_driver cs_config_driver = {

This set uses cs_cfg_ and cscfg_ to declare structures and function, making it very difficult to read the code. Please pick one (I'd go with the latter but that's a personal opinion) and stick with it.

Agreed - I started off using cs_cfg_.. for data structures, and cscfg_... for functions - but I'll use just the one as it is easier.

...

I also don't think the functionality provided in this file need to be a driver, it can simply be a set of functions handling configuration and features.

The thinking here was that:-

1. a virtual device is a good way of modelling functionality that

affects the entire coresight subsystem - i.e. treat it as a compound device. 2) A device object / kobject is required to support some kernel functionality such as sysfs and configfs when that is added.

That said, my work on configfs - which is not included in this set - would seem to indicate an explicit driver may not be needed, and a set of callback functions and data structures defined in the coresight module may be sufficient. So it may be possible to drop the driver abastaction for now and re-introduce if it does become necessary.

I think that makes sense and will help making this set smaller.

Thanks for the review

I will continue with patch 3 and 4 today.

Mike

...

...

• .drv = {
  

•         .name = "coresight-syscfg",
  

•         .owner = THIS_MODULE,
  

•         .suppress_bind_attrs = true,
  

•         .probe = cscfg_probe,
  

• },
  

+};

+/* called from coresight core init */ +int __init cscfg_driver_init(void) +{

• cs_config_driver.drv.bus = &coresight_bustype;
  

• return driver_register(&cs_config_driver.drv);
  

+}

+void cscfg_driver_exit(void) +{

• driver_unregister(&cs_config_driver.drv);
  

+} diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h new file mode 100644 index 000000000000..20ccca5e4151 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Coresight system configuration driver.
• */

+#ifndef CORESIGHT_SYSCFG_H +#define CORESIGHT_SYSCFG_H

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

+/**

• • System configuration device.
• • @csdev: The coresight device instance.
• • @dev_list: List of other coresight devices registered with this device.
• • @feat_list: List of features to load into registered devices.
• • @config_list:List of system configurations to load into registered devices.
• */

+struct cs_cfg_device {

• struct coresight_device csdev;
  

• struct list_head dev_list;
  

• struct list_head feat_list;
  

• struct list_head config_list;
  

• int nr_csdev;
  

+};

+/* basic device driver */ +struct cs_cfg_driver {

• struct device_driver    drv;
  

+};

+/* 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);

+#endif /* CORESIGHT_SYSCFG_H */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 7d3c87e5b97c..ed0a9d938303 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -42,6 +42,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_SOURCE, CORESIGHT_DEV_TYPE_HELPER, CORESIGHT_DEV_TYPE_ECT,

• CORESIGHT_DEV_TYPE_SYSCFG,
  

};

enum coresight_dev_subtype_sink {

2.17.1

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK