Re: [RFC PATCH v3 06/16] coresight: cti: Add device tree support for v8 arch CTI

On Wed, Jun 05, 2019 at 08:03:03PM +0100, Mike Leach wrote:

The v8 architecture defines the relationship between a PE, its optional ETM and a CTI. Unlike non-architectural CTIs which are implementation defined, this has a fixed set of connections which can therefore be represented as a simple tag in the device tree.

This patch defines the tags needed to create an entry for this PE/ETM/CTI relationship, and provides functionality to implement the connection model in the CTI driver.

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

.../coresight/coresight-cti-platform.c | 191 +++++++++++++++++- drivers/hwtracing/coresight/coresight-cti.c | 10 +- drivers/hwtracing/coresight/coresight-cti.h | 6 + 3 files changed, 200 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c index 068932ac9e90..2fb7d356e9c9 100644 --- a/drivers/hwtracing/coresight/coresight-cti-platform.c +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c @@ -7,8 +7,38 @@ #include "coresight-cti.h" -#ifdef CONFIG_OF +/* Number of CTI signals in the v8 architecturally defined connection */ +#define NR_V8PE_IN_SIGS 2 +#define NR_V8PE_OUT_SIGS 3 +#define NR_V8ETM_INOUT_SIGS 4

+/* CTI device tree connection property keywords */ +#define CTI_DT_V8ARCH "arm,cti-v8-arch" +#define CTI_DT_CSDEV_ASSOC "arm,cs-dev-assoc" +#define CTI_DT_CTM_ID "arm,cti-ctm-id"

+/*

• • Find a registered coresight device from a device fwnode.
• • The node info is associated with the AMBA parent, but the
• • csdev keeps a copy so iterate round the coresight bus to
• • find the device.
• */

+static struct coresight_device * +cti_get_assoc_csdev_by_fwnode(struct fwnode_handle *r_fwnode) +{

• struct device *dev;
• struct coresight_device *csdev = NULL;

• dev = bus_find_device(&coresight_bustype, NULL, r_fwnode,

• 	      coresight_device_fwnode_match);
  

Why not just use coresight_find_device_by_fwnode()?

• if (dev) {

• csdev = to_coresight_device(dev);
  

• put_device(dev);
  

• }
• return csdev;

+}

+#ifdef CONFIG_OF /*

• CTI can be bound to a CPU, or a system device.
• Reflect this in the return value and do not default to cpu 0

@@ -29,6 +59,157 @@ static int of_cti_get_cpu(const struct device_node *node) return (cpu < 0) ? -1 : cpu; } +static int of_cti_create_v8_etm_connection(struct cti_drvdata *drvdata,

• 			   struct device_node *np)
  

+{

• struct cti_trig_grp *etm_in = NULL, *etm_out = NULL;
• int *etm_sig_in_types = NULL, *etm_sig_out_types = NULL;
• int ret = 0, i;
• struct device_node *cs_np;
• const char *assoc_name = NULL;
• struct coresight_device *csdev;
• struct fwnode_handle *r_fwnode;
• /* Can optionally have an etm node - return if not */
• cs_np = of_parse_phandle(np, CTI_DT_CSDEV_ASSOC, 0);
• if (!cs_np)

• return 0;
  

• /* allocate memory */
• ret = -ENOMEM;
• etm_in = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!etm_in)

• goto of_create_v8_etm_out;
  

• etm_out = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!etm_out)

• goto of_create_v8_etm_out;
  

• etm_sig_in_types =

• kzalloc(sizeof(int) * NR_V8ETM_INOUT_SIGS, GFP_KERNEL);
  

• if (!etm_sig_in_types)

• goto of_create_v8_etm_out;
  

• etm_sig_out_types =

• kzalloc(sizeof(int) * NR_V8ETM_INOUT_SIGS, GFP_KERNEL);
  

• if (!etm_sig_out_types)

• goto of_create_v8_etm_out;
  

• /* build connection data */
• etm_in->nr_sigs = NR_V8ETM_INOUT_SIGS;
• etm_in->used_mask = 0xF0; /* sigs <4,5,6,7> */
• etm_in->sig_types = etm_sig_in_types;
• etm_out->nr_sigs = NR_V8ETM_INOUT_SIGS;
• etm_out->used_mask = 0xF0; /* sigs <4,5,6,7> */
• etm_out->sig_types = etm_sig_out_types;
• for (i = 0; i < NR_V8ETM_INOUT_SIGS; i++) {

• etm_sig_in_types[i] = CTITRIG_ETM_EXTOUT;
  

• etm_sig_out_types[i] = CTITRIG_ETM_EXTIN;
  

• }
• r_fwnode = of_fwnode_handle(cs_np);
• csdev = cti_get_assoc_csdev_by_fwnode(r_fwnode);
• if (csdev)

• assoc_name = dev_name(&csdev->dev);
  

• else

• assoc_name = cs_np->full_name;
  

• ret = cti_add_connection_entry(drvdata, etm_in, etm_out,

• 		       csdev, assoc_name);
  

+of_create_v8_etm_out:

• of_node_put(cs_np);
• if (ret) {

• kfree(etm_in);
  

• kfree(etm_out);
  

• kfree(etm_sig_in_types);
  

• kfree(etm_sig_out_types);
  

• }
• return ret;

+}

+/*

• • Create an architecturally defined v8 connection
• • must have a cpu, can have an ETM.
• */

+static int of_cti_create_v8_connections(struct device *dev,

• 			struct cti_drvdata *drvdata,
  

• 			struct device_node *np)
  

+{

• struct cti_device *cti_dev = &drvdata->ctidev;
• struct cti_trig_grp *in = NULL, *out = NULL;
• int cpuid = 0;
• char cpu_name_str[16];
• int *sig_in_types = NULL, *sig_out_types = NULL;
• int ret = -ENOMEM;
• /* Must have a cpu node */
• cpuid = of_cti_get_cpu(np);
• if (cpuid < 0) {

• dev_warn(dev, "CTI v8 DT binding no cpu\n");
  

• return -EINVAL;
  

• }
• cti_dev->cpu = cpuid;
• /* Allocate the v8 cpu memory */
• in = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!in)

• goto of_create_v8_out;
  

• out = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!out)

• goto of_create_v8_out;
  

• sig_in_types = kzalloc(sizeof(int) * NR_V8PE_IN_SIGS, GFP_KERNEL);
• if (!sig_in_types)

• goto of_create_v8_out;
  

• sig_out_types = kzalloc(sizeof(int) * NR_V8PE_OUT_SIGS, GFP_KERNEL);
• if (!sig_out_types)

• goto of_create_v8_out;
  

Here 'dev' is available, if devm_kzalloc() was to be used, it would make error handling much easier. The same applies to function of_cti_create_v8_etm_connection() and cti_add_default_connection().

• /* Create the v8 PE CTI connection */
• in->nr_sigs = NR_V8PE_IN_SIGS;
• in->used_mask = 0x3; /* sigs <0 1> */
• in->sig_types = sig_in_types;
• sig_in_types[0] = CTITRIG_PE_DBGTRIGGER;
• sig_in_types[1] = CTITRIG_PE_PMUIRQ;
• out->nr_sigs = NR_V8PE_OUT_SIGS;
• out->used_mask = 0x7; /* sigs <0 1 2 > */
• out->sig_types = sig_out_types;
• sig_out_types[0] = CTITRIG_PE_EDBGREQ;
• sig_out_types[1] = CTITRIG_PE_DBGRESTART;
• sig_out_types[2] = CTITRIG_PE_CTIIRQ;
• scnprintf(cpu_name_str, 16, "cpu%d", cpuid);
• ret = cti_add_connection_entry(drvdata, in, out, NULL, cpu_name_str);
• if (ret)

• goto of_create_v8_out;
  

• /* Create the v8 ETM associated connection */
• ret = of_cti_create_v8_etm_connection(drvdata, np);
• if (ret) {

• /*
  

•  * Need to remove PE connection just created which
  

•  * frees all associated memory.
  

•  */
  

• cti_free_conn_info(drvdata);
  

And if cti_add_connection_entry() was also using devm_kzalloc() and devm_kstrdup() you probably wouldn't need the above either, at least here.

• return ret;
  

• }
• /* filter pe_edbgreq - PE trigout sig <0> */
• drvdata->config.trig_out_filter |= 0x1;

+of_create_v8_out:

• if (ret) {

• kfree(in);
  

• kfree(out);
  

• kfree(sig_in_types);
  

• kfree(sig_out_types);
  

• }
• return ret;

+}

/* get the hardware configuration & connection data. */ int of_cti_get_hw_data(struct device *dev, struct device_node *np, @@ -37,6 +218,13 @@ int of_cti_get_hw_data(struct device *dev, int rc = 0; struct cti_device *cti_dev = &drvdata->ctidev;

• /* check for a v8 architectural CTI device */
• if (of_property_read_bool(np, CTI_DT_V8ARCH)) {

• rc = of_cti_create_v8_connections(dev, drvdata, np);
  

• if (rc)
  

• 	return rc;
  

• }
• /* if no connections, just add a single default based on max IN-OUT */ if (cti_dev->nr_trig_con == 0) rc = cti_add_default_connection(drvdata);

@@ -97,4 +285,3 @@ coresight_cti_get_platform_data(struct device *dev) error: return ERR_PTR(ret); }

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 817b6f70ee23..a4e349984ab8 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -214,11 +214,11 @@ static void cti_set_default_config(struct device *dev,

• Add a connection entry to the list of connections for this
• CTI device.

*/ -static int cti_add_connection_entry(struct cti_drvdata *drvdata,

• 		    struct cti_trig_grp *in_info,
  

• 		    struct cti_trig_grp *out_info,
  

• 		    struct coresight_device *csdev,
  

• 		    const char *assoc_dev_name)
  

+int cti_add_connection_entry(struct cti_drvdata *drvdata,

• 	     struct cti_trig_grp *in_info,
  

• 	     struct cti_trig_grp *out_info,
  

• 	     struct coresight_device *csdev,
  

• 	     const char *assoc_dev_name)
  

Nobody will call you out for not making the function static in 1/16 and adding the prototype to coresight-cti.h. If this was spread out over several patchset then making it static would be the way to go. But in this case it is work you've introduced in a previous patch that is done again, which we want to avoid.

Just make the function public in 1/16 along with the prototype in the header file. The same applies to cti_free_conn_info(), if based on the above devm_xyx() comments, it is still required..

{ struct cti_trig_con *tc; struct cti_device *cti_dev = &drvdata->ctidev; diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 33516bf378a8..b87cc67543e0 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -229,6 +229,12 @@ enum cti_chan_set_op { /* private cti driver fns & vars */ extern const struct attribute_group *coresight_cti_groups[]; int cti_add_default_connection(struct cti_drvdata *drvdata); +int cti_add_connection_entry(struct cti_drvdata *drvdata,

• 	     struct cti_trig_grp *in_info,
  

• 	     struct cti_trig_grp *out_info,
  

• 	     struct coresight_device *csdev,
  

• 	     const char *assoc_dev_name);
  

+void cti_free_conn_info(struct cti_drvdata *drvdata); int cti_enable(struct coresight_device *csdev, void *__unused); int cti_disable(struct coresight_device *csdev, void *__unused); void cti_write_all_hw_regs(struct cti_drvdata *drvdata); -- 2.20.1

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight