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

Hi Mathieu,

On Thu, 13 Jun 2019 at 21:34, Mathieu Poirier mathieu.poirier@linaro.org wrote:

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()?

Because that searches the AMBA bus, this function searches the CoreSight bus - so correctly locates coresight devices and therefore can get the related coresight_device structure

...

• 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().

prev set moved this to common code to create the trig_conn struct so re-written

...

• /* 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.

mem access updated to use devm for these items

...

•         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..

done

...

{ 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

Thanks

Mike

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