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

On 05/06/2019 20:03, 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);
  

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

Could we not use devm_kzalloc() everywhere above ? ^

Also, I assume this would change if you re-shuffle the struct definitions.

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

The used bits are 7-4, while the number of SIGS = 4. Does the nr_sigs need to be 8 and indicate that sigs 7-4 are only connected ?

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

• }

If you use devm_kzalloc(), you could ignore freeing them out in the error path.

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

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

s/16/sizeof(cpu_name_str) ?

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

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

I would prefer to use fwnode_property_* variants of the property accessors, wherever possible, thinking about the "ACPI" support picture. Of course, I understand that, we have not discussed about the bindings for the connections and stuff. But if we reuse the DT property names, we are all set for it, rather than a rework and we don't loose much here.

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

• if (rc)
  

• 	return rc;
  

• }

Suzuki