Hi Suzuki, On Fri, 29 Nov 2019 at 11:33, Suzuki Kuruppassery Poulose <suzuki.poulose@xxxxxxx> wrote: > > On 19/11/2019 23:19, 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@xxxxxxxxxx> > > --- > > .../coresight/coresight-cti-platform.c | 205 ++++++++++++++++++ > > 1 file changed, 205 insertions(+) > > > > diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c > > index 665be86c585d..790dd30b85f5 100644 > > --- a/drivers/hwtracing/coresight/coresight-cti-platform.c > > +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c > > @@ -3,10 +3,208 @@ > > * Copyright (c) 2019, The Linaro Limited. All rights reserved. > > */ > > > > +#include <dt-bindings/arm/coresight-cti-dt.h> > > #include <linux/of.h> > > > > #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" > > + > > +/* > > + * 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) > > To be frank this has nothing to do with the CTI and is in a way > a good candidate for a CoreSight generic function. We do similar > stuff in coresight_fixup_device_conns(). So this could be : > > struct coresight_device * > coresight_find_device_by_fwnode(const struct fwnode_handle *fwnode) > > > +{ > > + struct device *dev; > > + struct coresight_device *csdev = NULL; > > + > > + dev = bus_find_device_by_fwnode(&coresight_bustype, r_fwnode); > > + if (dev) { > > + csdev = to_coresight_device(dev); > > + put_device(dev); > > + } > > + return csdev; > > +} > > + > > And used in coresight_fixup_conns(). > OK - I'll look at that. > > +#ifdef CONFIG_OF > > +/* > > + * CTI can be bound to a CPU, or a system device. > > + * CPU can be declared at the device top level or in a connections node > > + * so need to check relative to node not device. > > + */ > > +static int of_cti_get_cpu_at_node(const struct device_node *node) > > +{ > > + int cpu; > > + struct device_node *dn; > > + > > + if (node == NULL) > > + return -1; > > + > > + dn = of_parse_phandle(node, "cpu", 0); > > + /* CTI affinity defaults to no cpu */ > > + if (!dn) > > + return -1; > > + cpu = of_cpu_node_to_id(dn); > > + of_node_put(dn); > > + > > + /* No Affinity if no cpu nodes are found */ > > + return (cpu < 0) ? -1 : cpu; > > +} > > + > > +static const char *of_cti_get_node_name(const struct device_node *node) > > +{ > > + if (node) > > + return node->full_name; > > + return "unknown"; > > +} > > +#else > > +static int of_cti_get_cpu_at_node(const struct device_node *node) > > +{ > > + return -1; > > +} > > + > > +static const char *of_cti_get_node_name(const struct device_node *node) > > +{ > > + return "unknown"; > > +} > > +#endif > > + > > +static int cti_plat_get_cpu_at_node(struct fwnode_handle *fwnode) > > +{ > > You may simply reuse coresight_get_cpu() below, instead of adding this > duplicate set of functions. See below. > > No we can't. coresight_get_cpu gets the 'cpu' entry relative to the device node, this gets the 'cpu' relative to the supplied node. This is very important for the case where a none v8 architected PE is attached to a CTI. This will use the devicetree form:- cti@<addr> { [ some stuff ] trig_conns@1 { cpu = <&CPU0> [trigger signal connection info for this cpu] } } trig_conns is a child node and we must look for 'cpu' relative to it. > > +static int cti_plat_create_v8_etm_connection(struct device *dev, > > + struct cti_drvdata *drvdata) > > +{ > > + int ret = -ENOMEM, i; > > + struct fwnode_handle *root_fwnode, *cs_fwnode; > > + const char *assoc_name = NULL; > > + struct coresight_device *csdev; > > + struct cti_trig_con *tc = NULL; > > + > > + root_fwnode = dev_fwnode(dev); > > + if (IS_ERR_OR_NULL(root_fwnode)) > > + return -EINVAL; > > + > > + /* Can optionally have an etm node - return if not */ > > + cs_fwnode = fwnode_find_reference(root_fwnode, CTI_DT_CSDEV_ASSOC, 0); > > + if (IS_ERR_OR_NULL(cs_fwnode)) > > + return 0; > > + > > + /* allocate memory */ > > + tc = cti_allocate_trig_con(dev, NR_V8ETM_INOUT_SIGS, > > + NR_V8ETM_INOUT_SIGS); > > + if (!tc) > > + goto create_v8_etm_out; > > + > > + /* build connection data */ > > + tc->con_in->used_mask = 0xF0; /* sigs <4,5,6,7> */ > > + tc->con_out->used_mask = 0xF0; /* sigs <4,5,6,7> */ > > + > > + /* > > + * The EXTOUT type signals from the ETM are connected to a set of input > > + * triggers on the CTI, the EXTIN being connected to output triggers. > > + */ > > + for (i = 0; i < NR_V8ETM_INOUT_SIGS; i++) { > > + tc->con_in->sig_types[i] = ETM_EXTOUT; > > + tc->con_out->sig_types[i] = ETM_EXTIN; > > + } > > + > > + /* > > + * We look to see if the ETM coresight device associated with this > > + * handle has been registered with the system - i.e. probed before > > + * this CTI. If so csdev will be non NULL and we can use the device > > + * name and pass the csdev to the connection entry function where > > + * the association will be recorded. > > + * If not, then simply record the name in the connection data, the > > + * probing of the ETM will call into the CTI driver API to update the > > + * association then. > > + */ > > + csdev = cti_get_assoc_csdev_by_fwnode(cs_fwnode); > > + if (csdev) > > + assoc_name = dev_name(&csdev->dev); > > Does it make sense to defer the probing until the ETM device turn up ? > Its fine either way. > Not really as the ETM is optional but the PE still has a CTI. > > + else > > + assoc_name = cti_plat_get_node_name(cs_fwnode); > > + ret = cti_add_connection_entry(dev, drvdata, tc, csdev, assoc_name); > > + > > +create_v8_etm_out: > > + fwnode_handle_put(cs_fwnode); > > + return ret; > > +} > > + > > +/* > > + * Create an architecturally defined v8 connection > > + * must have a cpu, can have an ETM. > > + */ > > +static int cti_plat_create_v8_connections(struct device *dev, > > + struct cti_drvdata *drvdata) > > +{ > > + struct cti_device *cti_dev = &drvdata->ctidev; > > + struct cti_trig_con *tc = NULL; > > + int cpuid = 0; > > + char cpu_name_str[16]; > > + int ret = -ENOMEM; > > + > > + /* Must have a cpu node */ > > + cpuid = cti_plat_get_cpu_at_node(dev_fwnode(dev)); > > Could we reuse coresight_get_cpu(dev) instead ? I understand that the > ACPI bindings have not been defined and it may be slightly different > from what we have now for the ETMs (i.e, ETM node as child of the CPU > node). But I don't see why we can't force it for the CTIs either. > In the worst case, you could still reuse the of_coresgith_get_cpu(dev) > instead of writing your own for the OF case. > See comments above - in theory here we could use coresight_get_cpu(), but for consistency it is better to use that same function throughout in case someone decided to "fix" it later. I probably need to beef up the comments around cti_plat_get_cpu_at_node / of_cti_get_cpu_at_node. > > > + if (cpuid < 0) { > > + dev_warn(dev, "CTI v8 DT binding no cpu\n"); > > This may be better off without mentioning the DT. e.g, > > "CTI Arm v8 architected connection: missing CPU\n" > OK > > > + return -EINVAL; > > + } > > + cti_dev->cpu = cpuid; > > + > > + /* Allocate the v8 cpu connection memory */ > > + tc = cti_allocate_trig_con(dev, NR_V8PE_IN_SIGS, NR_V8PE_OUT_SIGS); > > + if (!tc) > > + goto of_create_v8_out; > > + > > + /* Set the v8 PE CTI connection data */ > > + tc->con_in->used_mask = 0x3; /* sigs <0 1> */ > > + tc->con_in->sig_types[0] = PE_DBGTRIGGER; > > + tc->con_in->sig_types[1] = PE_PMUIRQ; > > + tc->con_out->used_mask = 0x7; /* sigs <0 1 2 > */ > > + tc->con_out->sig_types[0] = PE_EDBGREQ; > > + tc->con_out->sig_types[1] = PE_DBGRESTART; > > + tc->con_out->sig_types[2] = PE_CTIIRQ; > > + scnprintf(cpu_name_str, sizeof(cpu_name_str), "cpu%d", cpuid); > > + > > + ret = cti_add_connection_entry(dev, drvdata, tc, NULL, cpu_name_str); > > + if (ret) > > + goto of_create_v8_out; > > + > > + /* Create the v8 ETM associated connection */ > > + ret = cti_plat_create_v8_etm_connection(dev, drvdata); > > + if (ret) > > + goto of_create_v8_out; > > + > > + /* filter pe_edbgreq - PE trigout sig <0> */ > > + drvdata->config.trig_out_filter |= 0x1; > > + > > +of_create_v8_out: > > + return ret; > > +} > > + > > /* get the hardware configuration & connection data. */ > > int cti_plat_get_hw_data(struct device *dev, > > struct cti_drvdata *drvdata) > > @@ -14,6 +212,13 @@ int cti_plat_get_hw_data(struct device *dev, > > int rc = 0; > > struct cti_device *cti_dev = &drvdata->ctidev; > > > > + /* check for a v8 architectural CTI device */ > minor nit: Check for Arm v8 architected CTI connection ? > > > + if (device_property_read_bool(dev, CTI_DT_V8ARCH)) { > > + rc = cti_plat_create_v8_connections(dev, drvdata); > > + 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(dev, drvdata); > > > > > Suzuki Thanks Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK