Re: [RFC PATCH v2 03/13] drivers: coresight: cti: Add device tree support for v8 arch CTI

On Wed, May 01, 2019 at 09:49:35AM +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

drivers/hwtracing/coresight/coresight-cti.c | 10 +- drivers/hwtracing/coresight/coresight-cti.h | 5 + .../hwtracing/coresight/of_coresight-cti.c | 179 +++++++++++++++++- 3 files changed, 188 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 06b1f84795a4..32d9bf0bf0b9 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -192,11 +192,11 @@ static void cti_set_default_config(struct cti_config *config)

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

{ 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 a5bf59155f62..df40ca2d9548 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -234,6 +234,11 @@ enum cti_chan_set_op { 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);
  

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 379ca1113deb..ee9a5906dc37 100644 --- a/drivers/hwtracing/coresight/of_coresight-cti.c +++ b/drivers/hwtracing/coresight/of_coresight-cti.c @@ -18,6 +18,16 @@ #include "coresight-cti.h" +/* 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"

#ifdef CONFIG_OF /* @@ -40,6 +50,166 @@ static int of_cti_get_cpu(const struct device_node *node) return (cpu < 0) ? -1 : cpu; } +/* match function to pass to find_device */ +static int of_dev_node_match(struct device *dev, void *data) +{

• return dev->of_node == data;

+}

If I recall properly Suzuki has introduced that at the framework level as part of is his ACPI patchset.

+/*

• • Find a registered coresight device from the device_node.
• • The node info is associated with the AMBA parent, so we
• • search this first to find a name, and use the name to find
• • the device on the coresight bus as they are always named the same.
• */

+static struct coresight_device * +of_cti_get_assoc_cs_dev_by_node(struct device_node *node) +{

• struct device *dev, *adev;
• struct coresight_device *csdev = NULL;
• const char *amba_dev_name = NULL;
• if (!node)

• return NULL;
  

• adev = bus_find_device(&amba_bustype, NULL, node, of_dev_node_match);
• if (adev) {

• amba_dev_name = dev_name(adev);
  

• dev = bus_find_device_by_name(&coresight_bustype, NULL,
  

• 			      amba_dev_name);
  

• if (dev) {
  

• 	csdev = to_coresight_device(dev);
  

• 	put_device(dev);
  

• }
  

• put_device(adev);
  

• }
• return csdev;

+}

+/*

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

+static int of_cti_create_v8_connections(struct cti_drvdata *drvdata,

• 		       struct device_node *np)
  

Indentation problem

+{

• struct cti_device *cti_dev = &drvdata->ctidev;
• struct cti_trig_grp *in = 0, *out = 0, *etm_in = 0, *etm_out = 0;
• int cpuid = 0, i;
• struct device_node *cs_np;
• struct coresight_device *csdev = NULL;
• char cpu_name_str[16];
• const char *assoc_name = NULL;
• int *sig_in_types = 0, *sig_out_types = 0;
• int *etm_sig_in_types = 0, *etm_sig_out_types = 0;

I don't think the kernel community has a set rule for using '0' or NULL, but I'm pretty sure it leans toward the latter. In any case weaving both is guaranteed to be called out and should probably be remedied.

• /* Must have a cpu node */
• cpuid = of_cti_get_cpu(np);
• if (cpuid < 0) {

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

• return -EINVAL;
  

• }
• cti_dev->cpu = cpuid;
• /* Can have an etm node */
• cs_np = of_parse_phandle(np, CTI_DT_CSDEV_ASSOC, 0);
• /* Allocate the v8 cpu memory */
• in = kzalloc(sizeof(struct cti_trig_grp), GFP_KERNEL);
• if (!in)

• goto of_create_v8_mem_err;
  

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

• goto of_create_v8_mem_err;
  

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

• goto of_create_v8_mem_err;
  

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

• goto of_create_v8_mem_err;
  

• /* Allocate the ETM connection memory if required */
• if (cs_np) {

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

• if (!etm_in)
  

• 	goto of_create_v8_mem_err;
  

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

• if (!etm_out)
  

• 	goto of_create_v8_mem_err;
  

• etm_sig_in_types =
  

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

• if (!etm_sig_in_types)
  

• 	goto of_create_v8_mem_err;
  

• etm_sig_out_types =
  

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

• if (!etm_sig_out_types)
  

• 	goto of_create_v8_mem_err;
  

• }
• /* 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);
• cti_add_connection_entry(drvdata, in, out, NULL, cpu_name_str);

The error code for cti_add_connection_entry isn't checked.

• /* filter pe_edbgreq */
• drvdata->config.trig_out_filter = 0x1;
• /* Create the v8 ETM associated connection */
• if (cs_np) {

• etm_in->nr_sigs = NR_V8ETM_INOUT_SIGS;
  

• etm_in->used_mask = 0xF0;
  

• etm_in->sig_types = etm_sig_in_types;
  

• etm_out->nr_sigs = NR_V8ETM_INOUT_SIGS;
  

• etm_out->used_mask = 0xF0;
  

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

• }
  

• csdev = of_cti_get_assoc_cs_dev_by_node(cs_np);
  

• if (csdev)
  

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

• else
  

• 	assoc_name = cs_np->full_name;
  

I think the probing of this CTI should be delayed (-EPROBE_DEFER) if the device associated with cs_np hasn't been registered with the core framework.

• cti_add_connection_entry(drvdata, etm_in, etm_out,
  

• 			 csdev, assoc_name);
  

Same here, error code not checked.

• of_node_put(cs_np);
  

• }
• return 0;

+of_create_v8_mem_err:

• kfree(in);
• kfree(out);
• kfree(sig_in_types);
• kfree(sig_out_types);
• if (cs_np) {

• kfree(etm_in);
  

• kfree(etm_out);
  

• kfree(etm_sig_in_types);
  

• kfree(etm_sig_out_types);
  

• }
• return -ENOMEM;

+}

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

• /* get any CTM ID - defaults to 0 */
• of_property_read_u32(np, CTI_DT_CTM_ID, &cti_dev->ctm_id);

The above isn't related to the V8 architected CTI connections and should be moved elsewhere, preferably in a patch where it is used.

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

• rc = of_cti_create_v8_connections(drvdata, np);
  

The error code should be checked here and exit if something untoward happened. That will prevent having to deal with 'rc' below, which is a little messy.

• /* if no connections, just add a single default based on max IN-OUT */

• if (cti_dev->nr_trig_con == 0)

• if (!rc && (cti_dev->nr_trig_con == 0)) rc = cti_add_default_connection(drvdata); return rc;

}

2.20.1