Re: [RFC PATCH v2 04/13] drivers: coresight: cti: Add device tree support for impdef CTI.

HI Mathieu,

Spotted this just after I pressed the go button on the next revision. However the conversation doesn't change....

On Wed, 5 Jun 2019 at 18:05, Mathieu Poirier mathieu.poirier@linaro.org wrote:

Hi Mike,

Apologies on the late reply - I'm running out of cycles.

On Wed, 29 May 2019 at 06:17, Mike Leach mike.leach@linaro.org wrote:

...

Hi Mathieu,

On Fri, 10 May 2019 at 18:26, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

For the patch subject line in this set you can drop "drivers".

On Wed, May 01, 2019 at 09:49:36AM +0100, Mike Leach wrote:

...

Adds support for CTIs that are implementation defined at design time, and not constrained by v8 architecture.

Changelog descriptions needs to be line wrapped at 75 characters.

...

These CTIs have no standard connection setup, all the settings have to be defined in the device tree files. The patch creates a set of connections and trigger signals based on the information provided,

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

drivers/hwtracing/coresight/coresight-cti.h | 1 + .../hwtracing/coresight/of_coresight-cti.c | 191 +++++++++++++++++- 2 files changed, 191 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index df40ca2d9548..7342f998f4c5 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -239,6 +239,7 @@ int cti_add_connection_entry(struct cti_drvdata *drvdata, 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); diff --git a/drivers/hwtracing/coresight/of_coresight-cti.c b/drivers/hwtracing/coresight/of_coresight-cti.c index ee9a5906dc37..486fa99813ae 100644 --- a/drivers/hwtracing/coresight/of_coresight-cti.c +++ b/drivers/hwtracing/coresight/of_coresight-cti.c @@ -23,10 +23,19 @@ #define NR_V8PE_OUT_SIGS 3 #define NR_V8ETM_INOUT_SIGS 4

+/* CTI device tree trigger connection node keyword */ +#define CTI_DT_CONNS "trig-conns"

/* 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" +#define CTI_DT_TRIGIN_SIGS "arm,trig-in-sigs" +#define CTI_DT_TRIGOUT_SIGS "arm,trig-out-sigs" +#define CTI_DT_TRIGIN_TYPES "arm,trig-in-types" +#define CTI_DT_TRIGOUT_TYPES "arm,trig-out-types" +#define CTI_DT_FILTER_OUT_SIGS "arm,trig-filters" +#define CTI_DT_CONN_NAME "arm,trig-conn-name"

I will skip this patch while we don't have a firm idea of how we will represent CTI connections. It may be that we expand the graph binding for data component or come up with a whole new set, like this set does. There are pros and cons with both approach and I want the group to have a dicussion before moving forward.

Ok. But as I have mentioned before, the existing graph bindings have a built in assumption that all connections are coresight devices, and all the data is trace data. Graph bindings are not used between CPU and ETM - in fact there is no indication other than the cpu id that the connection exists at all. The information supplied here is that which is required to know how to program up the CTI device. Signal connection information alongside the device association information is a vital part of that. CPU=>ETM data flow is implied, CPU<=>CTI data flow needs to be explicit.

Any extension to the existing data path binding infrastructure needs so account for:- a) CoreSight devices - these are easy - we can put in / out nodes on both the device and CTI (given signal is bi-directional) and write coresight driver code to handle this/ b) non-Coresight devices that have an entry in the device tree (including CPUs) - can't really re-write arbitrary drivers to handle in / out nodes from coresight CTI, so this would have to be single ended. c) Any other device / IO pins that can connect but my not have a current entry in the device tree. Again this needs to be single ended. (for reference Juno has examples in all the above categories - so these are not hypothetical cases).

Using self contained connection information in the CTI device avoids having to add multiple in / out ports to lots of existing CoreSight definitions. For example, CTI0 connects to STM, TPIU, ETR and ETF0 - all bi-directionally. so thats 4 x in, 4 x out nodes on the CTI, plus (1 x in, 1 x out) x 4 devices. Then we have a choice as to where to put the signal information - at source, at sink, all on the associated device

• all at the CTI as now.

I am happy to consider suggestions as to a better way to do this, but I have considered and did try to force the existing data graph bindings to work. I simply wasn't happy with the direction it was going in.

This is a difficult problem - just like you did I tried expressing a CTI topology using our current graph bindings and things quickly became unmanageable. I am pretty sure that we will have to ask Rob Herring for guidance here. For now I suggest to concentrate on sysfs representation and deal with bindings in the next steps. As such when working on your next iteration please try to keep the code that reads bindings and the rest of the CTI driver as disjoint as possible so that one can be switched without affecting the other. If I remember correctly you're already doing well in that regard.

Indeed - the next set is platform agnostic & uses fwnodes, so it plays well with the APCI set on coresight/next. All the device tree stuff is now in the renamed coresight-cti-platform file, with a neutral API and room to add ACPI later.

Also, just like Suzuki did with his ACPI patchset see if you can split your work in two - one part that adds the CTI specific mechanic to probe, parse the DT and setup CTI devices and another to represent connections between signal components. We may simply end up extending the latter to represent connections between data components.

The key issue here is that it is very easy to program an ETM without knowing anything about what it is connected to. This programming task is self contained and independent of anything that might be in the connection information.

The CTI is complelely different in this regard. The connection information - particularly the trigger signal definitions are essential for the correct programming of the device. These affect both how the driver itself operates, and how any user will gain the insight required to program it. I am not sure there is such a clean distinction between CTI specifics and connections - programming the CTI directly interconnects the signals defined in these connections.

Regards

Mike

Thanks, Mathieu

...

Thanks

Mike

...

...

#ifdef CONFIG_OF

@@ -210,6 +219,181 @@ static int of_cti_create_v8_connections(struct cti_drvdata *drvdata, return -ENOMEM; }

+static int of_cti_read_trig_group(struct cti_trig_grp **trig_grp_ptr,

•                           struct device_node *np,
  

•                           const char *grp_name,
  

•                           int max_trigs)
  

+{

• int items, err = 0;
  

• u32 value, pidx;
  

• struct cti_trig_grp *grp;
  

• grp = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
  

• if (!grp)
  

•         return -ENOMEM;
  

• *trig_grp_ptr = grp;
  

• items = of_property_count_elems_of_size(np, grp_name, 4);
  

• /* if the property doesn't exist or has no values, then return
  

•  * an empty connection group
  

•  */
  

• if (items < 0)
  

•         return 0;
  

• if (items > max_trigs)
  

•         return -EINVAL;
  

• /* set the number of signals and usage mask */
  

• for (pidx = 0; pidx < items; pidx++) {
  

•         err = of_property_read_u32_index(np, grp_name, pidx, &value);
  

•         if (err)
  

•                 return err;
  

•         grp->nr_sigs++;
  

•         grp->used_mask |= (0x1 << value);
  

• }
  

• return 0;
  

+}

+static int of_cti_read_trig_types(struct cti_trig_grp *trig_grp_ptr,

•                           struct device_node *np,
  

•                            const char *type_name)
  

+{

• int items, used = 0, err = 0, nr_sigs;
  

• u32 value, i;
  

• /* allocate an array according to number of signals in connection */
  

• nr_sigs = trig_grp_ptr->nr_sigs;
  

• if (!nr_sigs)
  

•         return 0;
  

• trig_grp_ptr->sig_types = kzalloc(sizeof(int) * nr_sigs, GFP_KERNEL);
  

• if (!trig_grp_ptr->sig_types)
  

•         return -ENOMEM;
  

• /* see if any types have been included in the device description */
  

• items = of_property_count_elems_of_size(np, type_name, 4);
  

• /*
  

•  * Match type id to signal index,
  

•  * too few - default to genio,
  

•  * too many - ignore excess.
  

•  */
  

• for (i = 0; i < nr_sigs; i++) {
  

•         if (used < items) {
  

•                 err = of_property_read_u32_index(np, type_name,
  

•                                                  i, &value);
  

•                 if (!err)
  

•                         trig_grp_ptr->sig_types[i] = value;
  

•                 else
  

•                         trig_grp_ptr->sig_types[i] = CTITRIG_GEN_IO;
  

•                 used++;
  

•         }
  

•         else
  

•                 trig_grp_ptr->sig_types[i] = CTITRIG_GEN_IO;
  

• }
  

• return 0;
  

+}

+static void of_cti_free_trig_grp(struct cti_trig_grp *grp) +{

• if (grp) {
  

•         kfree(grp->sig_types);
  

•         kfree(grp);
  

• }
  

+}

+static int of_cti_create_connection(struct device *dev,

•                             struct device_node *np,
  

•                             struct cti_drvdata *drvdata)
  

+{

• struct cti_trig_grp *in = 0, *out = 0, *filter = 0;
  

• int cpuid = -1, err = 0, trig_max = drvdata->config.nr_trig_max;
  

• struct device_node *cs_np;
  

• struct coresight_device *csdev = NULL;
  

• const char *assoc_name = "unknown";
  

• char cpu_name_str[16];
  

• /* look for the signals properties. */
  

• err = of_cti_read_trig_group(&in, np, CTI_DT_TRIGIN_SIGS, trig_max);
  

• if (err)
  

•         goto of_create_con_err;
  

• err = of_cti_read_trig_types(in, np, CTI_DT_TRIGIN_TYPES);
  

• if (err)
  

•         goto of_create_con_err;
  

• err = of_cti_read_trig_group(&out, np, CTI_DT_TRIGOUT_SIGS, trig_max);
  

• if (err)
  

•         goto of_create_con_err;
  

• err = of_cti_read_trig_types(out, np, CTI_DT_TRIGOUT_TYPES);
  

• if (err)
  

•         goto of_create_con_err;
  

• err = of_cti_read_trig_group(&filter, np, CTI_DT_FILTER_OUT_SIGS,
  

•                              trig_max);
  

• if (err)
  

•         goto of_create_con_err;
  

• /* read the connection name if set - may be overridden by later */
  

• of_property_read_string(np, CTI_DT_CONN_NAME, &assoc_name);
  

• /* associated cpu ? */
  

• cpuid = of_cti_get_cpu(np);
  

• if (cpuid >= 0) {
  

•         drvdata->ctidev.cpu = cpuid;
  

•         scnprintf(cpu_name_str, 16, "cpu%d", cpuid);
  

•         assoc_name = cpu_name_str;
  

• } else {
  

•         /* associated device ? */
  

•         cs_np = of_parse_phandle(np,  CTI_DT_CSDEV_ASSOC, 0);
  

•         if (cs_np) {
  

•                 csdev = of_cti_get_assoc_cs_dev_by_node(cs_np);
  

•                 if (csdev) /* use device name if csdev found */
  

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

•                 else  /* otherwise node name for later association */
  

•                         assoc_name = cs_np->full_name;
  

•                 of_node_put(cs_np);
  

•         }
  

• }
  

• /* set up a connection */
  

• err = cti_add_connection_entry(drvdata, in, out, csdev, assoc_name);
  

• if (err)
  

•         goto of_create_con_err;
  

• /* note any filter info */
  

• drvdata->config.trig_out_filter |= filter->used_mask;
  

• kfree(filter);
  

• return err;
  

+of_create_con_err:

• of_cti_free_trig_grp(in);
  

• of_cti_free_trig_grp(out);
  

• of_cti_free_trig_grp(filter);
  

• return err;
  

+}

+static int of_cti_create_impdef_connections(struct device *dev,

•               struct device_node *np,
  

•               struct cti_drvdata *drvdata)
  

+{

• int rc = 0;
  

• struct device_node *nc = NULL;
  

• for_each_child_of_node(np, nc) {
  

•         if (of_node_cmp(nc->name, CTI_DT_CONNS) != 0)
  

•                 continue;
  

•         rc = of_cti_create_connection(dev, nc, drvdata);
  

•         if (rc != 0)
  

•                 goto of_cti_impdef_create_err;
  

• }
  

• return rc;
  

+of_cti_impdef_create_err:

• /* last create should have freed its mem - unpick previous */
  

• cti_free_conn_info(drvdata);
  

• return rc;
  

+}

/* get the hardware configuration & connection data. */ int of_cti_get_hw_data(struct device *dev, struct device_node *np, @@ -221,9 +405,14 @@ int of_cti_get_hw_data(struct device *dev, /* get any CTM ID - defaults to 0 */ of_property_read_u32(np, CTI_DT_CTM_ID, &cti_dev->ctm_id);

• /* check for a v8 architectural CTI device */
  

• /*
  

•  * Check for a v8 architectural CTI device,
  

•  * otherwise implementation defined.
  

•  */
  if (of_property_read_bool(np, CTI_DT_V8ARCH))
          rc = of_cti_create_v8_connections(drvdata, np);
  

• else
  

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

-- 2.20.1

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