Re: [RFC PATCH v3 07/16] coresight: cti: Add device tree support for impdef CTI.

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

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

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

.../coresight/coresight-cti-platform.c | 202 +++++++++++++++++- 1 file changed, 197 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c index 2fb7d356e9c9..806941e1011e 100644 --- a/drivers/hwtracing/coresight/coresight-cti-platform.c +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c @@ -12,10 +12,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"

This is declared in the previous file but used here. Please move the declaration here.

+#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" /*

• Find a registered coresight device from a device fwnode.

@@ -210,6 +219,182 @@ static int of_cti_create_v8_connections(struct device *dev, return ret; } +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);

Once again I think we can use devm_kzalloc() to simplify the error path.

• if (!grp)

• return -ENOMEM;
  

• *trig_grp_ptr = grp;

Memory allocation needs to be decoupled from reading of the signals. Otherwise of_cti_read_trig_group() _has_ to be called before of_cti_read_trig_types().

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

Memory leak.

• if (items > max_trigs)

• return -EINVAL;
  

Here too.

• /* 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 = kcalloc(nr_sigs, sizeof(int), 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);

Is there a use case for describing the signals but not their type? If not nr_sigs != items is an error.

• /*

• * Match type id to signal index,
  

• * too few - default to genio,
  

• * too many - ignore excess.
  

• */
  

I think we should return an error in such cases.

• 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 cti_drvdata *drvdata,
  

• 		    struct device_node *np)
  

+{

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

NULL

• 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];
• struct fwnode_handle *r_fwnode;
• /* look for the signals properties. */
• err = of_cti_read_trig_group(&in, np, CTI_DT_TRIGIN_SIGS, trig_max);

Function of_cti_read_trig_group() is dealing with signals, I would call it of_cti_read_trig_sig().

• 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) {
  

• 	r_fwnode = of_fwnode_handle(cs_np);
  

• 	csdev = cti_get_assoc_csdev_by_fwnode(r_fwnode);
  

• 	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 cti_drvdata *drvdata,
  

• 			    struct device_node *np)
  

+{

• 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, drvdata, nc);
  

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

As much as I try, I don't understand the comment - please revise.

• 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, @@ -218,12 +403,19 @@ 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)) {

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

• * otherwise implementation defined.
  

• */
  

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

• if (rc)
  

• 	return rc;
  

• }

• else

• rc = of_cti_create_impdef_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)

Now that I have a clearer picture of how things work, wouldn't it be simpler to mandate that all connections be specified in the DT? Even if we are to add default connections via cti_add_default_connection(), there is no telling the HW lines are there. As such we could simply return an error if rc == 0. Otherwise I'm missing a use case.

-- 2.20.1

---

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