CoreSight November 2021

coresight@lists.linaro.org

8 participants
31 discussions

Re: [PATCH 04/10] Coresight: Enable BC and GPR for TPDM driver

by Mathieu Poirier

On Fri, Nov 05, 2021 at 04:17:54PM +0800, Jinlong wrote: > On Thu, Nov 04, 2021 at 11:02:24AM -0600, Mathieu Poirier wrote: > > [...] > > > > > > > + > > > > > +static ssize_t reset_store(struct device *dev, > > > > > + struct device_attribute *attr, > > > > > + const char *buf, > > > > > + size_t size) > > > > > +{ > > > > > + int ret = 0; > > > > > + unsigned long val; > > > > > + struct mcmb_dataset *mcmb_temp = NULL; > > > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > > > + > > > > > + ret = kstrtoul(buf, 10, &val); > > > > > > > > The coresight subsystem normally uses the hexadecimal base. > > > > > > > > > > We will address you comments. > > > > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Shouldn't this be "if (!ret)" ? > > > > > > > > > > When ret is not 0, it need to return. > > > > I would expect something like this: > > > > $ echo 1 > /sys/path/to/tpdm/device/reset > > > > and not > > > > $ echo 0 > /sys/path/to/tpdm/device/reset > > > > The latter is what the code does. > > > > Thanks, > > Mathieu > > > > Hi Mathieu, > > The ret is the result of kstrtoul not the val. > Ah yes, you are correct.

3 years, 1 month

Re: [PATCH 05/10] Coresight: Add interface for TPDM BC subunit

by Mathieu Poirier

On Thu, Oct 21, 2021 at 03:38:51PM +0800, Tao Zhang wrote: > The BC(Basic Counters) interface has RW, WO and RO fields for > controlling BC dataset elements transmitted on ATB flush. > The BC data set subunit supports from 1-32 counter instances > allowing for collection of BC data sets. > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 873 +++++++++++++++++++ > 1 file changed, 873 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index c0a01979e42f..0970c69ac8e2 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -668,6 +668,878 @@ static ssize_t gp_regs_store(struct device *dev, > } > static DEVICE_ATTR_RW(gp_regs); > > +static ssize_t bc_capture_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) Indentation. I won't repeat this comment but please make sure it is fixed for the entire patchset. > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%s\n", > + drvdata->bc->capture_mode == TPDM_MODE_ATB ? > + "ATB" : "APB"); > +} > + > +static ssize_t bc_capture_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + char str[20] = ""; char str[4]; > + uint32_t val; > + > + if (size >= 20) > + return -EINVAL; > + if (sscanf(buf, "%s", str) != 1) if (sscanf(buf, "%3s", str) != 1) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; return -EINVAL; Please make sure this is fixed everywhere, except when -EINVAL is really the right error code. > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; Why does the device need to be enabled for this operation? Again no comments... > + } > + > + if (!strcmp(str, "ATB")) { > + drvdata->bc->capture_mode = TPDM_MODE_ATB; > + } else if (!strcmp(str, "APB") && > + drvdata->bc->retrieval_mode == TPDM_MODE_APB) { > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + val = val | BIT(3); > + tpdm_writel(drvdata, val, TPDM_BC_CR); > + TPDM_LOCK(drvdata); > + > + drvdata->bc->capture_mode = TPDM_MODE_APB; > + } else { > + mutex_unlock(&drvdata->lock); > + return -EINVAL; > + } > + > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_capture_mode); > + > +static ssize_t bc_retrieval_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%s\n", > + drvdata->bc->retrieval_mode == TPDM_MODE_ATB ? > + "ATB" : "APB"); > +} > + > +static ssize_t bc_retrieval_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + char str[20] = ""; > + > + if (size >= 20) > + return -EINVAL; > + if (sscanf(buf, "%s", str) != 1) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; Same here, I don't know why the device needs to be enabled for this to success. Please fix everywhere. > + } > + > + if (!strcmp(str, "ATB")) { > + drvdata->bc->retrieval_mode = TPDM_MODE_ATB; > + } else if (!strcmp(str, "APB")) { > + drvdata->bc->retrieval_mode = TPDM_MODE_APB; > + } else { > + mutex_unlock(&drvdata->lock); > + return -EINVAL; > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_retrieval_mode); > + > +static ssize_t bc_reset_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + val = val | BIT(1); > + tpdm_writel(drvdata, val, TPDM_BC_CR); > + TPDM_LOCK(drvdata); > + } > + > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_WO(bc_reset_counters); > + > +static ssize_t bc_sat_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->sat_mode); return scnprintf(buf, PAGE_SIZE, "%#x\n", drvdata->bc->sat_mode); And everywhere casting in used... > +} > + > +static ssize_t bc_sat_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->sat_mode = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_sat_mode); > + > +static ssize_t bc_enable_counters_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_counters); > +} > + > +static ssize_t bc_enable_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_counters = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_counters); > + > +static ssize_t bc_clear_counters_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->clear_counters); > +} > + > +static ssize_t bc_clear_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->clear_counters = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_clear_counters); > + > +static ssize_t bc_enable_irq_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_irq); > +} > + > +static ssize_t bc_enable_irq_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_irq = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_irq); > + > +static ssize_t bc_clear_irq_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->clear_irq); > +} > + > +static ssize_t bc_clear_irq_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->clear_irq = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_clear_irq); > + > +static ssize_t bc_trig_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->trig_val_lo[i]); As previously stated, the sysfs interface should output single line and single values. I won't comment on this again, please fix everywhere. > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_trig_val_lo_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= drvdata->bc_counters_avail || > + drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO || > + (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0)) > + return -EPERM; > This is hard to read and maintain. Please break it up in multiple if() statements. > + mutex_lock(&drvdata->lock); > + drvdata->bc->trig_val_lo[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_trig_val_lo); > + > +static ssize_t bc_trig_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->trig_val_hi[i]); > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_trig_val_hi_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= drvdata->bc_counters_avail || > + drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO || > + (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->trig_val_hi[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_trig_val_hi); > + > +static ssize_t bc_enable_ganging_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_ganging); > +} > + > +static ssize_t bc_enable_ganging_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_ganging = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_ganging); > + > +static ssize_t bc_overflow_val_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_OVERFLOW; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->overflow_val[i]); > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_overflow_val_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= TPDM_BC_MAX_OVERFLOW) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->overflow_val[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_overflow_val); > + > +static ssize_t bc_ovsr_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_OVSR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_ovsr_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_OVSR); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_ovsr); > + > +static ssize_t bc_counter_sel_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_SELR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_counter_sel_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable || val >= drvdata->bc_counters_avail) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_SELR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_counter_sel); > + > +static ssize_t bc_count_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CNTR_LO); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_count_val_lo_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val, select; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { if (!val) { mutex_unlock(&drvdata->lock); return -EINVAL; } > + TPDM_UNLOCK(drvdata); > + select = tpdm_readl(drvdata, TPDM_BC_SELR); > + > + /* Check if selected counter is disabled */ > + if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + tpdm_writel(drvdata, val, TPDM_BC_CNTR_LO); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_count_val_lo); > + > +static ssize_t bc_count_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CNTR_HI); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_count_val_hi_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val, select; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { Same > + TPDM_UNLOCK(drvdata); > + select = tpdm_readl(drvdata, TPDM_BC_SELR); > + > + /* Check if selected counter is disabled */ > + if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + tpdm_writel(drvdata, val, TPDM_BC_CNTR_HI); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_count_val_hi); > + > +static ssize_t bc_shadow_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + for (i = 0; i < drvdata->bc_counters_avail; i++) { > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + tpdm_readl(drvdata, TPDM_BC_SHADOW_LO(i))); > + } > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RO(bc_shadow_val_lo); > + > +static ssize_t bc_shadow_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + for (i = 0; i < drvdata->bc_counters_avail; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + tpdm_readl(drvdata, TPDM_BC_SHADOW_HI(i))); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RO(bc_shadow_val_hi); > + > +static ssize_t bc_sw_inc_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_SWINC); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_sw_inc_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_SWINC); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_sw_inc); > + > +static ssize_t bc_msr_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int i; > + ssize_t len = 0; > + > + if (!drvdata->msr_support) > + return -EINVAL; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + for (i = 0; i < TPDM_BC_MAX_MSR; i++) > + len += scnprintf(buf + len, PAGE_SIZE - len, "%u 0x%x\n", > + i, drvdata->bc->msr[i]); > + > + return len; > +} > + > +static ssize_t bc_msr_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int num, val; > + int nval; > + > + if (!drvdata->msr_support) > + return -EINVAL; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + nval = sscanf(buf, "%u %x", &num, &val); > + if (nval != 2) > + return -EINVAL; > + > + if (num >= TPDM_BC_MAX_MSR) > + return -EINVAL; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->msr[num] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_msr); > + > +static struct attribute *tpdm_bc_attrs[] = { > + &dev_attr_bc_capture_mode.attr, > + &dev_attr_bc_retrieval_mode.attr, > + &dev_attr_bc_reset_counters.attr, > + &dev_attr_bc_sat_mode.attr, > + &dev_attr_bc_enable_counters.attr, > + &dev_attr_bc_clear_counters.attr, > + &dev_attr_bc_enable_irq.attr, > + &dev_attr_bc_clear_irq.attr, > + &dev_attr_bc_trig_val_lo.attr, > + &dev_attr_bc_trig_val_hi.attr, > + &dev_attr_bc_enable_ganging.attr, > + &dev_attr_bc_overflow_val.attr, > + &dev_attr_bc_ovsr.attr, > + &dev_attr_bc_counter_sel.attr, > + &dev_attr_bc_count_val_lo.attr, > + &dev_attr_bc_count_val_hi.attr, > + &dev_attr_bc_shadow_val_lo.attr, > + &dev_attr_bc_shadow_val_hi.attr, > + &dev_attr_bc_sw_inc.attr, > + &dev_attr_bc_msr.attr, > + NULL, This will result in a very crowded directory. Please move under a "bc" subdirectory. And as I commented before, all sysfs entries need to be documented under Documentation/ABI/testing. > +}; > + > +static struct attribute_group tpdm_bc_attr_grp = { > + .attrs = tpdm_bc_attrs, > +}; > + > static struct attribute *tpdm_attrs[] = { > &dev_attr_available_datasets.attr, > &dev_attr_enable_datasets.attr, > @@ -682,6 +1554,7 @@ static struct attribute_group tpdm_attr_grp = { > }; > static const struct attribute_group *tpdm_attr_grps[] = { > &tpdm_attr_grp, > + &tpdm_bc_attr_grp, It is quite tedious to review all these options at the same time as the core drivers. I suggest to concentrate on the base functionality for now. When that is merged we can add configuration options such as these. I am out of time for this patchset and as such will not review the remaining patches - those will have to wait for another iteration. Thanks, Mathieu > NULL, > }; > > -- > 2.17.1 >

3 years, 1 month

Re: [PATCH 04/10] Coresight: Enable BC and GPR for TPDM driver

by Mathieu Poirier

[...] > > > + > > > +static ssize_t reset_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, > > > + size_t size) > > > +{ > > > + int ret = 0; > > > + unsigned long val; > > > + struct mcmb_dataset *mcmb_temp = NULL; > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > + > > > + ret = kstrtoul(buf, 10, &val); > > > > The coresight subsystem normally uses the hexadecimal base. > > > > We will address you comments. > > > > + if (ret) > > > + return ret; > > > > Shouldn't this be "if (!ret)" ? > > > > When ret is not 0, it need to return. I would expect something like this: $ echo 1 > /sys/path/to/tpdm/device/reset and not $ echo 0 > /sys/path/to/tpdm/device/reset The latter is what the code does. Thanks, Mathieu > > > > + > > > + mutex_lock(&drvdata->lock); > > > + /* Reset all datasets to ZERO */ > > > + if (drvdata->gpr != NULL) > > > + memset(drvdata->gpr, 0, sizeof(struct gpr_dataset)); > > > + > > > + if (drvdata->bc != NULL) > > > + memset(drvdata->bc, 0, sizeof(struct bc_dataset)); > > > + > > > + if (drvdata->tc != NULL) > > > + memset(drvdata->tc, 0, sizeof(struct tc_dataset)); > > > + > > > + if (drvdata->dsb != NULL) > > > + memset(drvdata->dsb, 0, sizeof(struct dsb_dataset)); > > > + > > > + if (drvdata->cmb != NULL) { > > > + if (drvdata->cmb->mcmb != NULL) { > > > + mcmb_temp = drvdata->cmb->mcmb; > > > + memset(drvdata->cmb->mcmb, 0, > > > + sizeof(struct mcmb_dataset)); > > > + } > > > + > > > + memset(drvdata->cmb, 0, sizeof(struct cmb_dataset)); > > > + drvdata->cmb->mcmb = mcmb_temp; > > > + } > > > + /* Init the default data */ > > > + tpdm_init_default_data(drvdata); > > > + > > > + mutex_unlock(&drvdata->lock); > > > + > > > + /* Disable tpdm if enabled */ > > > + if (drvdata->enable) > > > + coresight_disable(drvdata->csdev); > > > > Why is this done out of the lock? > > > > When call coresight_disable, tpdm_disable will be called. There is lock in tpdm_disable. > If add it into the lock, there will be dead lock. > > > > + > > > + return size; > > > +} > > > +static DEVICE_ATTR_WO(reset); > > > + > > > +static ssize_t integration_test_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, > > > + size_t size) > > > +{ > > > + int i, ret = 0; > > > + unsigned long val; > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > + > > > + ret = kstrtoul(buf, 10, &val); > > > + if (ret) > > > + return ret; > > > + > > > + if (val != 1 && val != 2) > > > + return -EINVAL; > > > + > > > + if (!drvdata->enable) > > > + return -EINVAL; > > > + > > > + if (val == 1) > > > + val = ATBCNTRL_VAL_64; > > > + else > > > + val = ATBCNTRL_VAL_32; > > > + TPDM_UNLOCK(drvdata); > > > + tpdm_writel(drvdata, 0x1, TPDM_ITCNTRL); > > > + > > > + for (i = 1; i < 5; i++) > > > + tpdm_writel(drvdata, val, TPDM_ITATBCNTRL); > > > + > > > + tpdm_writel(drvdata, 0, TPDM_ITCNTRL); > > > + TPDM_LOCK(drvdata); > > > + return size; > > > +} > > > +static DEVICE_ATTR_WO(integration_test); > > > > Integration test interface should be conditional to a compile time option. Have > > a look at what was done for CTIs. > > > > We will check and update. > > > > + > > > +static ssize_t gp_regs_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > + ssize_t size = 0; > > > + int i = 0; > > > + > > > + if (!test_bit(TPDM_DS_GPR, drvdata->datasets)) > > > + return -EPERM; > > > > return -EINVAL; > > > > > + > > > + mutex_lock(&drvdata->lock); > > > + for (i = 0; i < TPDM_GPR_REGS_MAX; i++) { > > > + if (!test_bit(i, drvdata->gpr->gpr_dirty)) > > > + continue; > > > + size += scnprintf(buf + size, PAGE_SIZE - size, > > > + "Index: 0x%x Value: 0x%x\n", i, > > > + drvdata->gpr->gp_regs[i]); > > > > This should not be - the sysfs interface requires outputs of a single line. > > > > We will check and update. > > > > + } > > > + mutex_unlock(&drvdata->lock); > > > + return size; > > > +} > > > + > > > +static ssize_t gp_regs_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, > > > + size_t size) > > > +{ > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > + unsigned long index, val; > > > + > > > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > > > + return -EINVAL; > > > + if (!test_bit(TPDM_DS_GPR, drvdata->datasets) || > > > + index >= TPDM_GPR_REGS_MAX) > > > + return -EPERM; > > > + > > > + mutex_lock(&drvdata->lock); > > > + drvdata->gpr->gp_regs[index] = val; > > > + __set_bit(index, drvdata->gpr->gpr_dirty); > > > + mutex_unlock(&drvdata->lock); > > > + return size; > > > +} > > > +static DEVICE_ATTR_RW(gp_regs); > > > + > > > +static struct attribute *tpdm_attrs[] = { > > > + &dev_attr_available_datasets.attr, > > > + &dev_attr_enable_datasets.attr, > > > + &dev_attr_reset.attr, > > > + &dev_attr_integration_test.attr, > > > + &dev_attr_gp_regs.attr, > > > + NULL, > > > +}; > > > > All new sysfs interface need to be documented. See here: > > > > Documentation/ABI/testing/sysfs-bus-coresight-devices-xyz > > > > More comments to come... > > > > We will add the comments. > > > Thanks, > > Mathieu > > > > > + > > > +static struct attribute_group tpdm_attr_grp = { > > > + .attrs = tpdm_attrs, > > > +}; > > > +static const struct attribute_group *tpdm_attr_grps[] = { > > > + &tpdm_attr_grp, > > > + NULL, > > > +}; > > > + > > > static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata) > > > { > > > if (test_bit(TPDM_DS_GPR, drvdata->datasets)) { > > > @@ -513,6 +846,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > > > desc.ops = &tpdm_cs_ops; > > > desc.pdata = adev->dev.platform_data; > > > desc.dev = &adev->dev; > > > + desc.groups = tpdm_attr_grps; > > > drvdata->csdev = coresight_register(&desc); > > > if (IS_ERR(drvdata->csdev)) > > > return PTR_ERR(drvdata->csdev); > > > -- > > > 2.17.1 > > >

3 years, 1 month

Re: [PATCH 03/10] Coresight: Add driver to support Coresight device TPDM

by Mathieu Poirier

[...] > > > > + > > > +#ifdef CONFIG_CORESIGHT_TPDM_DEFAULT_ENABLE > > > +static int boot_enable = 1; > > > +#else > > > +static int boot_enable; > > > +#endif > > > > That isn't the proper way to do this. Look at how it is done in > > coresight-etm4x.c > > > > > + > > > +struct gpr_dataset { > > > + DECLARE_BITMAP(gpr_dirty, TPDM_GPR_REGS_MAX); > > > + uint32_t gp_regs[TPDM_GPR_REGS_MAX]; > > > > Shouldn't this be u32? > > > > uint32_t is the same as u32. > typedef u32 uint32_t; Right - but the common kernel convention is to use u64/32/16/8. Please refactor for the entire patchset. > > > > +}; > > > + > > > +struct bc_dataset { > > > + enum tpdm_mode capture_mode; > > > + enum tpdm_mode retrieval_mode; > > > + uint32_t sat_mode; > > > + uint32_t enable_counters; > > > + uint32_t clear_counters; > > > + uint32_t enable_irq; > > > + uint32_t clear_irq; > > > + uint32_t trig_val_lo[TPDM_BC_MAX_COUNTERS]; > > > + uint32_t trig_val_hi[TPDM_BC_MAX_COUNTERS]; > > > + uint32_t enable_ganging; > > > + uint32_t overflow_val[TPDM_BC_MAX_OVERFLOW]; > > > + uint32_t msr[TPDM_BC_MAX_MSR]; > > > +}; > > > + > > > +struct tc_dataset { > > > + enum tpdm_mode capture_mode; > > > + enum tpdm_mode retrieval_mode; > > > + bool sat_mode; > > > + uint32_t enable_counters; > > > + uint32_t clear_counters; > > > + uint32_t enable_irq; > > > + uint32_t clear_irq; > > > + uint32_t trig_sel[TPDM_TC_MAX_TRIG]; > > > + uint32_t trig_val_lo[TPDM_TC_MAX_TRIG]; > > > + uint32_t trig_val_hi[TPDM_TC_MAX_TRIG]; > > > + uint32_t msr[TPDM_TC_MAX_MSR]; > > > +}; > > > + > > > +struct dsb_dataset { > > > + uint32_t mode; > > > + uint32_t edge_ctrl[TPDM_DSB_MAX_EDCR]; > > > + uint32_t edge_ctrl_mask[TPDM_DSB_MAX_EDCR / 2]; > > > + uint32_t patt_val[TPDM_DSB_MAX_PATT]; > > > + uint32_t patt_mask[TPDM_DSB_MAX_PATT]; > > > + bool patt_ts; > > > + bool patt_type; > > > + uint32_t trig_patt_val[TPDM_DSB_MAX_PATT]; > > > + uint32_t trig_patt_mask[TPDM_DSB_MAX_PATT]; > > > + bool trig_ts; > > > + bool trig_type; > > > + uint32_t select_val[TPDM_DSB_MAX_SELECT]; > > > + uint32_t msr[TPDM_DSB_MAX_MSR]; > > > +}; > > > + > > > +struct mcmb_dataset { > > > + uint8_t mcmb_trig_lane; > > > + uint8_t mcmb_lane_select; > > > +}; > > > + > > > +struct cmb_dataset { > > > + bool trace_mode; > > > + uint32_t cycle_acc; > > > + uint32_t patt_val[TPDM_CMB_PATT_CMP]; > > > + uint32_t patt_mask[TPDM_CMB_PATT_CMP]; > > > + bool patt_ts; > > > + uint32_t trig_patt_val[TPDM_CMB_PATT_CMP]; > > > + uint32_t trig_patt_mask[TPDM_CMB_PATT_CMP]; > > > + bool trig_ts; > > > + bool ts_all; > > > + uint32_t msr[TPDM_CMB_MAX_MSR]; > > > + uint8_t read_ctl_reg; > > > + struct mcmb_dataset *mcmb; > > > +}; > > > + > > > +DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); > > > + > > > +struct tpdm_drvdata { > > > + void __iomem *base; > > > + struct device *dev; > > > + struct coresight_device *csdev; > > > + int nr_tclk; > > > + struct clk **tclk; > > > + int nr_treg; > > > + struct regulator **treg; > > > + struct mutex lock; > > > + bool enable; > > > + bool clk_enable; > > > + DECLARE_BITMAP(datasets, TPDM_DATASETS); > > > + DECLARE_BITMAP(enable_ds, TPDM_DATASETS); > > > + enum tpdm_support_type tc_trig_type; > > > + enum tpdm_support_type bc_trig_type; > > > + enum tpdm_support_type bc_gang_type; > > > + uint32_t bc_counters_avail; > > > + uint32_t tc_counters_avail; > > > + struct gpr_dataset *gpr; > > > + struct bc_dataset *bc; > > > + struct tc_dataset *tc; > > > + struct dsb_dataset *dsb; > > > + struct cmb_dataset *cmb; > > > + int traceid; > > > + uint32_t version; > > > + bool msr_support; > > > + bool msr_fix_req; > > > + bool cmb_msr_skip; > > > +}; > > > > All of these should also be in a header file and properly documented. > > > > We will move these to header file and add the documentation. > > > > + > > > +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata); > > > > This isn't needed. > > We will remove this. > > > > > + > > > +static void __tpdm_enable(struct tpdm_drvdata *drvdata) > > > +{ > > > + TPDM_UNLOCK(drvdata); > > > + > > > + if (drvdata->clk_enable) > > > + tpdm_writel(drvdata, 0x1, TPDM_CLK_CTRL); > > > + > > > + TPDM_LOCK(drvdata); > > > +} > > > + > > > +static int tpdm_enable(struct coresight_device *csdev, > > > + struct perf_event *event, u32 mode) > > > +{ > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > > + int ret = 0; > > > + > > > + if (drvdata->enable) { > > > > This a race condition. > > > > Need to add it to mutex_lock. We will update. > > > > + dev_err(drvdata->dev, > > > + "TPDM setup already enabled,Skipping enablei\n"); > > > > Please remove this. > > > > We will remove this. > > > > + return ret; > > > > Shouldn't this be return -EBUSY? > > We will address your comments. > > > > > > + } > > > + > > > + mutex_lock(&drvdata->lock); > > > + __tpdm_enable(drvdata); > > > + drvdata->enable = true; > > > + mutex_unlock(&drvdata->lock); > > > + > > > + dev_info(drvdata->dev, "TPDM tracing enabled\n"); > > > + return 0; > > > +} > > > + > > > +static void __tpdm_disable(struct tpdm_drvdata *drvdata) > > > +{ > > > + TPDM_UNLOCK(drvdata); > > > + > > > + if (drvdata->clk_enable) > > > + tpdm_writel(drvdata, 0x0, TPDM_CLK_CTRL); > > > + > > > + TPDM_LOCK(drvdata); > > > +} > > > + > > > +static void tpdm_disable(struct coresight_device *csdev, > > > + struct perf_event *event) > > > +{ > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > > + > > > + if (!drvdata->enable) { > > > + dev_err(drvdata->dev, > > > + "TPDM setup already disabled, Skipping disable\n"); > > > + return; > > > + } > > > + mutex_lock(&drvdata->lock); > > > + __tpdm_disable(drvdata); > > > + drvdata->enable = false; > > > + mutex_unlock(&drvdata->lock); > > > > Same comments as above. > > > > We will address your comments. > > > > + > > > + dev_info(drvdata->dev, "TPDM tracing disabled\n"); > > > +} > > > + > > > +static int tpdm_trace_id(struct coresight_device *csdev) > > > +{ > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > > + > > > + return drvdata->traceid; > > > +} > > > + > > > +static const struct coresight_ops_source tpdm_source_ops = { > > > + .trace_id = tpdm_trace_id, > > > + .enable = tpdm_enable, > > > + .disable = tpdm_disable, > > > +}; > > > + > > > +static const struct coresight_ops tpdm_cs_ops = { > > > + .source_ops = &tpdm_source_ops, > > > +}; > > > + > > > +static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata) > > > +{ > > > + if (test_bit(TPDM_DS_GPR, drvdata->datasets)) { > > > + drvdata->gpr = devm_kzalloc(drvdata->dev, sizeof(*drvdata->gpr), > > > + GFP_KERNEL); > > > + if (!drvdata->gpr) > > > + return -ENOMEM; > > > + } > > > + if (test_bit(TPDM_DS_BC, drvdata->datasets)) { > > > + drvdata->bc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->bc), > > > + GFP_KERNEL); > > > + if (!drvdata->bc) > > > + return -ENOMEM; > > > + } > > > + if (test_bit(TPDM_DS_TC, drvdata->datasets)) { > > > + drvdata->tc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->tc), > > > + GFP_KERNEL); > > > + if (!drvdata->tc) > > > + return -ENOMEM; > > > + } > > > + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) { > > > + drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb), > > > + GFP_KERNEL); > > > + if (!drvdata->dsb) > > > + return -ENOMEM; > > > + } > > > + if (test_bit(TPDM_DS_CMB, drvdata->datasets)) { > > > + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb), > > > + GFP_KERNEL); > > > + if (!drvdata->cmb) > > > + return -ENOMEM; > > > + } else if (test_bit(TPDM_DS_MCMB, drvdata->datasets)) { > > > + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb), > > > + GFP_KERNEL); > > > + if (!drvdata->cmb) > > > + return -ENOMEM; > > > + drvdata->cmb->mcmb = devm_kzalloc(drvdata->dev, > > > + sizeof(*drvdata->cmb->mcmb), > > > + GFP_KERNEL); > > > + if (!drvdata->cmb->mcmb) > > > + return -ENOMEM; > > > > How can I understand what the above does when: > > > > 1) There isn't a single line of comments. > > 2) I don't know the HW. > > 3) I don't have access to the documentation. > > > > We will add comments here. > > > > + } > > > + return 0; > > > +} > > > + > > > +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) > > > +{ > > > + if (test_bit(TPDM_DS_BC, drvdata->datasets)) > > > + drvdata->bc->retrieval_mode = TPDM_MODE_ATB; > > > + > > > + if (test_bit(TPDM_DS_TC, drvdata->datasets)) > > > + drvdata->tc->retrieval_mode = TPDM_MODE_ATB; > > > + > > > + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) { > > > + drvdata->dsb->trig_ts = true; > > > + drvdata->dsb->trig_type = false; > > > + } > > > + > > > + if (test_bit(TPDM_DS_CMB, drvdata->datasets) || > > > + test_bit(TPDM_DS_MCMB, drvdata->datasets)) > > > + drvdata->cmb->trig_ts = true; > > > +} > > > + > > > +static int tpdm_parse_of_data(struct tpdm_drvdata *drvdata) > > > +{ > > > + int i, ret; > > > + const char *tclk_name, *treg_name; > > > + struct device_node *node = drvdata->dev->of_node; > > > + > > > + drvdata->clk_enable = of_property_read_bool(node, "qcom,clk-enable"); > > > + drvdata->msr_fix_req = of_property_read_bool(node, "qcom,msr-fix-req"); > > > + drvdata->cmb_msr_skip = of_property_read_bool(node, > > > + "qcom,cmb-msr-skip"); > > > + > > > + drvdata->nr_tclk = of_property_count_strings(node, "qcom,tpdm-clks"); > > > + if (drvdata->nr_tclk > 0) { > > > + drvdata->tclk = devm_kzalloc(drvdata->dev, drvdata->nr_tclk * > > > + sizeof(*drvdata->tclk), > > > + GFP_KERNEL); > > > + if (!drvdata->tclk) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < drvdata->nr_tclk; i++) { > > > + ret = of_property_read_string_index(node, > > > + "qcom,tpdm-clks", i, &tclk_name); > > > + if (ret) > > > + return ret; > > > + > > > + drvdata->tclk[i] = devm_clk_get(drvdata->dev, > > > + tclk_name); > > > + if (IS_ERR(drvdata->tclk[i])) > > > + return PTR_ERR(drvdata->tclk[i]); > > > + } > > > + } > > > + > > > + drvdata->nr_treg = of_property_count_strings(node, "qcom,tpdm-regs"); > > > + if (drvdata->nr_treg > 0) { > > > + drvdata->treg = devm_kzalloc(drvdata->dev, drvdata->nr_treg * > > > + sizeof(*drvdata->treg), > > > + GFP_KERNEL); > > > + if (!drvdata->treg) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < drvdata->nr_treg; i++) { > > > + ret = of_property_read_string_index(node, > > > + "qcom,tpdm-regs", i, &treg_name); > > > + if (ret) > > > + return ret; > > > + > > > + drvdata->treg[i] = devm_regulator_get(drvdata->dev, > > > + treg_name); > > > + if (IS_ERR(drvdata->treg[i])) > > > + return PTR_ERR(drvdata->treg[i]); > > > + } > > > + } > > > > _None_ of the above are defined in the yaml file and/or part of the example that > > is shown there. Moreover they don't appear in patch 10/10 where TPDM and TPDA are > > supposed to be introduced. I will comment on patch 10/10 when I'm done with > > this one. > > > > We will update the dtsi change in next version. > > > > + > > > + return 0; > > > +} > > > + > > > +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > > > +{ > > > + int ret, i; > > > + uint32_t pidr, devid; > > > + struct device *dev = &adev->dev; > > > + struct coresight_platform_data *pdata; > > > + struct tpdm_drvdata *drvdata; > > > + struct coresight_desc desc = { 0 }; > > > + static int traceid = TPDM_TRACE_ID_START; > > > + uint32_t version; > > > + > > > + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); > > > + if (!desc.name) > > > + return -ENOMEM; > > > + pdata = coresight_get_platform_data(dev); > > > + if (IS_ERR(pdata)) > > > + return PTR_ERR(pdata); > > > + adev->dev.platform_data = pdata; > > > + > > > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > > > + if (!drvdata) > > > + return -ENOMEM; > > > + drvdata->dev = &adev->dev; > > > + dev_set_drvdata(dev, drvdata); > > > + > > > + drvdata->base = devm_ioremap_resource(dev, &adev->res); > > > + if (!drvdata->base) > > > + return -ENOMEM; > > > + > > > + mutex_init(&drvdata->lock); > > > + > > > + ret = tpdm_parse_of_data(drvdata); > > > + if (ret) { > > > + dev_err(drvdata->dev, "TPDM parse of data fail\n"); > > > + return -EINVAL; > > > + } > > > + > > > + desc.type = CORESIGHT_DEV_TYPE_SOURCE; > > > + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_PROC; > > > > Why is this of subtype CORESIGHT_DEV_SUBTYPE_SOURCE_PROC when TPDMs are not > > associated with a CPU? Here we should probably introduce something like > > CORESIGHT_DEV_SUBTYPE_SOURCE_SYS > > > We will check and update. > > > > > > + desc.ops = &tpdm_cs_ops; > > > + desc.pdata = adev->dev.platform_data; > > > + desc.dev = &adev->dev; > > > + drvdata->csdev = coresight_register(&desc); > > > + if (IS_ERR(drvdata->csdev)) > > > + return PTR_ERR(drvdata->csdev); > > > + > > > + version = tpdm_readl(drvdata, CORESIGHT_PERIPHIDR2); > > > + drvdata->version = BMVAL(version, 4, 7); > > > + > > > > What is MSR support? This should be documented. Looking more closely at this > > patch, not a single line of documentation is provided. If you look at other > > drivers, you will find things to be quite different. The coresight subsystem > > has become very complex and documentation helps us, and anyone trying to > > contribute, understand what the code does. > > > > We will add more documentation in the driver. > > > > > + if (drvdata->version) > > > + drvdata->msr_support = true; > > > + > > > + pidr = tpdm_readl(drvdata, CORESIGHT_PERIPHIDR0); > > > + for (i = 0; i < TPDM_DATASETS; i++) { > > > + if (pidr & BIT(i)) { > > > + __set_bit(i, drvdata->datasets); > > > + __set_bit(i, drvdata->enable_ds); > > > + } > > > + } > > > + > > > + ret = tpdm_datasets_alloc(drvdata); > > > + if (ret) { > > > + coresight_unregister(drvdata->csdev); > > > + return ret; > > > + } > > > + > > > + tpdm_init_default_data(drvdata); > > > + > > > + devid = tpdm_readl(drvdata, CORESIGHT_DEVID); > > > + drvdata->tc_trig_type = BMVAL(devid, 27, 28); > > > + drvdata->bc_trig_type = BMVAL(devid, 25, 26); > > > + drvdata->bc_gang_type = BMVAL(devid, 23, 24); > > > + drvdata->bc_counters_avail = BMVAL(devid, 6, 10) + 1; > > > + drvdata->tc_counters_avail = BMVAL(devid, 4, 5) + 1; > > > > Shouldn't this be part of tpdm_init_default_data()? > > > > We will address your comments. > > > > + > > > + drvdata->traceid = traceid++; > > > > For this to work a new function needs to be introduced in coresight-pmu.h. > > Something like: > > > > static inline int coresight_get_system_trace_id(int id) > > { > > /* Start system IDs above the highest per CPU trace ID. */ > > return coresigth_get_trace_id(cpumask_last(cpu_possible_mask) + 1); > > } > > > > We will address your comments. > > > > + > > > + dev_dbg(drvdata->dev, "TPDM initialized\n"); > > > > Please remove this. > > > > We will remove it. > > > > + > > > + if (boot_enable) > > > + coresight_enable(drvdata->csdev); > > > + > > > + pm_runtime_put(&adev->dev); > > > + > > > + return 0; > > > +} > > > + > > > +static struct amba_id tpdm_ids[] = { > > > + { > > > + .id = 0x001f0e00, > > > + .mask = 0x00ffff00, > > > > Any way we can use CS_AMBA_ID() here? > > > > We will address your comments. > > > > + .data = "TPDM", > > > > What is .data used for? > > It is just to fill the structure. We will remove it if it is not necessary. > > > > > > + }, > > > + { 0, 0}, > > > +}; > > > + > > > +static struct amba_driver tpdm_driver = { > > > + .drv = { > > > + .name = "coresight-tpdm", > > > + .owner = THIS_MODULE, > > > + .suppress_bind_attrs = true, > > > + }, > > > + .probe = tpdm_probe, > > > + .id_table = tpdm_ids, > > > +}; > > > + > > > +module_amba_driver(tpdm_driver); > > > + > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); > > > -- > > > 2.17.1 > > >

3 years, 1 month

[PATCH] arm64: cpufeature: Export this_cpu_has_cap helper

by Suzuki K Poulose

From: Arnd Bergmann <arnd(a)arndb.de> Export the this_cpu_has_cap() for use by modules. This is used by TRBE driver. Without this patch, TRBE will fail to build as a module : ERROR: modpost: "this_cpu_has_cap" [drivers/hwtracing/coresight/coresight-trbe.ko] undefined! Fixes: 8a1065127d95 ("coresight: trbe: Add infrastructure for Errata handling") Cc: Will Deacon <will(a)kernel.org> Cc: Catalin Marinas <catalin.marinas(a)arm.com> Cc: Mathieu Poirier <mathieu.poirier(a)linaro.org> Cc: Anshuman Khandual <anshuman.khandual(a)arm.com> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> [ change to EXPORT_SYMBOL_GPL ] Acked-by: Catalin Marinas <catalin.marinas(a)arm.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> --- arch/arm64/kernel/cpufeature.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index f8a3067d10c6..82e68c69bb99 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -2839,6 +2839,7 @@ bool this_cpu_has_cap(unsigned int n) return false; } +EXPORT_SYMBOL_GPL(this_cpu_has_cap); /* * This helper function is used in a narrow window when, -- 2.25.4

3 years, 1 month

Re: [PATCH 10/10] ARM: dts: msm: Add TPDA and TPDM support to DTS for RB5

by Suzuki K Poulose

On 21/10/2021 08:38, Tao Zhang wrote: > Add TPDA and TPDM support to DTS for RB5 board. This change is a > sample for validating. After applying this patch, the new TPDM and > TPDA nodes can be observed at the coresight devices path. TPDM and > TPDA hardware can be operated by commands. > > List the commands for validating this series patches as below. > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm0/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm0/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm0.bin > echo 0 > /sys/bus/coresight/devices/tpdm0/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm1/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm1/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm1/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm1.bin > echo 0 > /sys/bus/coresight/devices/tpdm1/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm2/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm2/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm2/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm2.bin > echo 0 > /sys/bus/coresight/devices/tpdm2/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > > If the data from TPDMs can be obtained from the ETF, it means > that the TPDMs verification is successful. At the same time, How can we decode the TPDM trace ? Is there a public decoder available ? > since TPDM0, TPDM1 and TPDM2 are all connected to the same > funnel "funnel@6c2d000" and output via different output ports, > it also means that the following patches verification is > successful. > coresight: add support to enable more coresight paths > coresight: funnel: add support for multiple output ports > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 439 +++++++++++++++++++++++ > 1 file changed, 439 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index 8ac96f8e79d4..bcec8b181e11 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -222,6 +222,445 @@ > + > + funnel@6b04000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + arm,primecell-periphid = <0x000bb908>; > + > + reg = <0 0x6b04000 0 0x1000>; > + reg-names = "funnel-base"; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + merge_funnel_out: endpoint { > + remote-endpoint = > + <&etf_in>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@7 { > + reg = <7>; > + swao_funnel_in7: endpoint { > + slave-mode; This is obsolete, with the new in-ports/out-ports construct. Suzuki

3 years, 1 month

Re: [PATCH 03/10] Coresight: Add driver to support Coresight device TPDM

by Mathieu Poirier

Good morning, On Thu, Oct 21, 2021 at 03:38:49PM +0800, Tao Zhang wrote: > Add driver to support Coresight device TPDM. This driver provides > support for configuring monitor. Monitors are primarily responsible > for data set collection and support the ability to collect any > permutation of data set types. Monitors are also responsible for > interaction with system cross triggering. As far as I can tell there is nothing related to CTIs in this patch. And if there is, it is not documented. > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > .../bindings/arm/coresight-tpdm.yaml | 86 +++ As checkpatch says, this should be in a separate file. > MAINTAINERS | 5 + Since this is a coresight device Suzuki and I will continue the maintenance. The get_maintainer script will make sure you care CC'ed on patches related to the TPDM/TPDA drivers, and we would typically requried a "Reviewed-by" tag from you before merging. > drivers/hwtracing/coresight/Kconfig | 9 + > drivers/hwtracing/coresight/Makefile | 1 + > drivers/hwtracing/coresight/coresight-tpdm.c | 583 ++++++++++++++++++ > 5 files changed, 684 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml > create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c > > diff --git a/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml > new file mode 100644 > index 000000000000..44541075d77f > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml > @@ -0,0 +1,86 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/arm/coresight-tpdm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Trace, Profiling and Diagnostics Monitor - TPDM > + > +description: | > + The TPDM or Monitor serves as data collection component for various dataset > + types specified in the QPMDA spec. It covers Basic Counts (BC), Tenure > + Counts (TC), Continuous Multi-Bit (CMB), and Discrete Single Bit (DSB). It > + performs data collection in the data producing clock domain and transfers it > + to the data collection time domain, generally ATB clock domain. > + > + The primary use case of the TPDM is to collect data from different data > + sources and send it to a TPDA for packetization, timestamping, and funneling. > + > +maintainers: > + - Tao Zhang <quic_taozha(a)quicinc.com> > + > +properties: > + $nodename: > + pattern: "^tpdm(@[0-9a-f]+)$" > + compatible: > + items: > + - const: arm,primecell > + > + reg: > + maxItems: 1 > + > + reg-names: > + items: > + - const: tpdm-base > + > + atid: > + maxItems: 1 > + description: | > + The QPMDA specification repurposed the ATID field of the AMBA ATB > + specification to use it to convey packetization information to the > + Aggregator. > + > + out-ports: > + description: | > + Output connections from the TPDM to legacy CoreSight trace bus. > + $ref: /schemas/graph.yaml#/properties/ports > + properties: > + port: > + description: Output connection from the TPDM to legacy CoreSight > + Trace bus. > + $ref: /schemas/graph.yaml#/properties/port > + > +required: > + - compatible > + - reg > + - reg-names > + - atid > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + # minimum TPDM definition. > + - | > + tpdm@6980000 { > + compatible = "arm,primecell"; > + reg = <0x6980000 0x1000>; > + reg-names = "tpdm-base"; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + atid = <78>; > + out-ports { > + port { > + tpdm_turing_out_funnel_turing: endpoint { > + remote-endpoint = > + <&funnel_turing_in_tpdm_turing>; > + }; > + }; > + }; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index eeb4c70b3d5b..cabecf760488 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15303,6 +15303,11 @@ L: netdev(a)vger.kernel.org > S: Supported > F: drivers/net/ipa/ > > +QCOM CORESIGHT TPDM DRIVER > +M: Tao Zhang <quic_taozha(a)quicinc.com> > +S: Maintained > +F: drivers/hwtracing/coresight/coresight-tpdm.c > + > QEMU MACHINE EMULATOR AND VIRTUALIZER SUPPORT > M: Gabriel Somlo <somlo(a)cmu.edu> > M: "Michael S. Tsirkin" <mst(a)redhat.com> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > index f026e5c0e777..abe244a968f6 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -188,4 +188,13 @@ config CORESIGHT_TRBE > > To compile this driver as a module, choose M here: the module will be > called coresight-trbe. > + > +config CORESIGHT_TPDM > + tristate "CoreSight Trace, Profiling & Diagnostics Monitor driver" > + select CORESIGHT_LINKS_AND_SINKS > + help > + This driver provides support for configuring monitor. Monitors are > + primarily responsible for data set collection and support the > + ability to collect any permutation of data set types. Monitors are > + also responsible for interaction with system cross triggering. You also need to indicate what the name of the module would be. Please look at other extries in the Kconfig file for examples. > endif > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > index b6c4a48140ec..e7392a0dddeb 100644 > --- a/drivers/hwtracing/coresight/Makefile > +++ b/drivers/hwtracing/coresight/Makefile > @@ -25,5 +25,6 @@ obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o > obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o > obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o > obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o > +obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o > coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ > coresight-cti-sysfs.o > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > new file mode 100644 > index 000000000000..906776c859d6 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -0,0 +1,583 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/amba/bus.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/fs.h> > +#include <linux/bitmap.h> > +#include <linux/of.h> > +#include <linux/coresight.h> > +#include <linux/regulator/consumer.h> These should be in alphabetical order. > + > +#include "coresight-priv.h" > + > +#define tpdm_writel(drvdata, val, off) __raw_writel((val), drvdata->base + off) > +#define tpdm_readl(drvdata, off) __raw_readl(drvdata->base + off) writel/readl_relaxed() are defines for __raw_writel/readl and as such the above should not be needed. > + > +#define TPDM_LOCK(drvdata) \ > +do { \ > + mb(); /* ensure configuration take effect before we lock it */ \ > + tpdm_writel(drvdata, 0x0, CORESIGHT_LAR); \ > +} while (0) > +#define TPDM_UNLOCK(drvdata) \ > +do { \ > + tpdm_writel(drvdata, CORESIGHT_UNLOCK, CORESIGHT_LAR); \ > + mb(); /* ensure unlock take effect before we configure */ \ > +} while (0) That too isn't needed. Simply use CS_LOCK/UNLOCK(). > + > +/* GPR Registers */ > +#define TPDM_GPR_CR(n) (0x0 + (n * 4)) > + > +/* BC Subunit Registers */ > +#define TPDM_BC_CR (0x280) > +#define TPDM_BC_SATROLL (0x284) > +#define TPDM_BC_CNTENSET (0x288) > +#define TPDM_BC_CNTENCLR (0x28C) > +#define TPDM_BC_INTENSET (0x290) > +#define TPDM_BC_INTENCLR (0x294) > +#define TPDM_BC_TRIG_LO(n) (0x298 + (n * 4)) > +#define TPDM_BC_TRIG_HI(n) (0x318 + (n * 4)) > +#define TPDM_BC_GANG (0x398) > +#define TPDM_BC_OVERFLOW(n) (0x39C + (n * 4)) > +#define TPDM_BC_OVSR (0x3C0) > +#define TPDM_BC_SELR (0x3C4) > +#define TPDM_BC_CNTR_LO (0x3C8) > +#define TPDM_BC_CNTR_HI (0x3CC) > +#define TPDM_BC_SHADOW_LO(n) (0x3D0 + (n * 4)) > +#define TPDM_BC_SHADOW_HI(n) (0x450 + (n * 4)) > +#define TPDM_BC_SWINC (0x4D0) > +#define TPDM_BC_MSR(n) (0x4F0 + (n * 4)) > + > +/* TC Subunit Registers */ > +#define TPDM_TC_CR (0x500) > +#define TPDM_TC_CNTENSET (0x504) > +#define TPDM_TC_CNTENCLR (0x508) > +#define TPDM_TC_INTENSET (0x50C) > +#define TPDM_TC_INTENCLR (0x510) > +#define TPDM_TC_TRIG_SEL(n) (0x514 + (n * 4)) > +#define TPDM_TC_TRIG_LO(n) (0x534 + (n * 4)) > +#define TPDM_TC_TRIG_HI(n) (0x554 + (n * 4)) > +#define TPDM_TC_OVSR_GP (0x580) > +#define TPDM_TC_OVSR_IMPL (0x584) > +#define TPDM_TC_SELR (0x588) > +#define TPDM_TC_CNTR_LO (0x58C) > +#define TPDM_TC_CNTR_HI (0x590) > +#define TPDM_TC_SHADOW_LO(n) (0x594 + (n * 4)) > +#define TPDM_TC_SHADOW_HI(n) (0x644 + (n * 4)) > +#define TPDM_TC_SWINC (0x700) > +#define TPDM_TC_MSR(n) (0x768 + (n * 4)) > + > +/* DSB Subunit Registers */ > +#define TPDM_DSB_CR (0x780) > +#define TPDM_DSB_TIER (0x784) > +#define TPDM_DSB_TPR(n) (0x788 + (n * 4)) > +#define TPDM_DSB_TPMR(n) (0x7A8 + (n * 4)) > +#define TPDM_DSB_XPR(n) (0x7C8 + (n * 4)) > +#define TPDM_DSB_XPMR(n) (0x7E8 + (n * 4)) > +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) > +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) > +#define TPDM_DSB_CA_SELECT(n) (0x86c + (n * 4)) > +#define TPDM_DSB_MSR(n) (0x980 + (n * 4)) > + > +/* CMB/MCMB Subunit Registers */ > +#define TPDM_CMB_CR (0xA00) > +#define TPDM_CMB_TIER (0xA04) > +#define TPDM_CMB_TPR(n) (0xA08 + (n * 4)) > +#define TPDM_CMB_TPMR(n) (0xA10 + (n * 4)) > +#define TPDM_CMB_XPR(n) (0xA18 + (n * 4)) > +#define TPDM_CMB_XPMR(n) (0xA20 + (n * 4)) > +#define TPDM_CMB_MARKR (0xA28) > +#define TPDM_CMB_READCTL (0xA70) > +#define TPDM_CMB_READVAL (0xA74) > +#define TPDM_CMB_MSR(n) (0xA80 + (n * 4)) > + > +/* TPDM Specific Registers */ > +#define TPDM_ITATBCNTRL (0xEF0) > +#define TPDM_CLK_CTRL (0x220) > +#define TPDM_ITCNTRL (0xF00) > + > + > +#define TPDM_DATASETS 32 > +#define TPDM_BC_MAX_COUNTERS 32 > +#define TPDM_BC_MAX_OVERFLOW 6 > +#define TPDM_BC_MAX_MSR 4 > +#define TPDM_TC_MAX_COUNTERS 44 > +#define TPDM_TC_MAX_TRIG 8 > +#define TPDM_TC_MAX_MSR 6 > +#define TPDM_DSB_MAX_PATT 8 > +#define TPDM_DSB_MAX_SELECT 8 > +#define TPDM_DSB_MAX_MSR 32 > +#define TPDM_DSB_MAX_EDCR 16 > +#define TPDM_DSB_MAX_LINES 256 > +#define TPDM_CMB_PATT_CMP 2 > +#define TPDM_CMB_MAX_MSR 128 > +#define TPDM_MCMB_MAX_LANES 8 > + > +/* DSB programming modes */ > +#define TPDM_DSB_MODE_CYCACC(val) BMVAL(val, 0, 2) > +#define TPDM_DSB_MODE_PERF BIT(3) > +#define TPDM_DSB_MODE_HPBYTESEL(val) BMVAL(val, 4, 8) > +#define TPDM_MODE_ALL (0xFFFFFFF) > + > +#define NUM_OF_BITS 32 > +#define TPDM_GPR_REGS_MAX 160 > + > +#define TPDM_TRACE_ID_START 128 > + > +#define TPDM_REVISION_A 0 > +#define TPDM_REVISION_B 1 > + > +#define HW_ENABLE_CHECK_VALUE 0x10 > + > + Extra newline > +#define ATBCNTRL_VAL_32 0xC00F1409 > +#define ATBCNTRL_VAL_64 0xC01F1409 > + All of the above should be in a header file. > + > +enum tpdm_dataset { > + TPDM_DS_IMPLDEF, > + TPDM_DS_DSB, > + TPDM_DS_CMB, > + TPDM_DS_TC, > + TPDM_DS_BC, > + TPDM_DS_GPR, > + TPDM_DS_MCMB, > +}; > + > +enum tpdm_mode { > + TPDM_MODE_ATB, > + TPDM_MODE_APB, > +}; > + > +enum tpdm_support_type { > + TPDM_SUPPORT_TYPE_FULL, > + TPDM_SUPPORT_TYPE_PARTIAL, > + TPDM_SUPPORT_TYPE_NO, > +}; > + > +enum tpdm_cmb_patt_bits { > + TPDM_CMB_LSB, > + TPDM_CMB_MSB, > +}; These too. > + > +#ifdef CONFIG_CORESIGHT_TPDM_DEFAULT_ENABLE > +static int boot_enable = 1; > +#else > +static int boot_enable; > +#endif That isn't the proper way to do this. Look at how it is done in coresight-etm4x.c > + > +struct gpr_dataset { > + DECLARE_BITMAP(gpr_dirty, TPDM_GPR_REGS_MAX); > + uint32_t gp_regs[TPDM_GPR_REGS_MAX]; Shouldn't this be u32? > +}; > + > +struct bc_dataset { > + enum tpdm_mode capture_mode; > + enum tpdm_mode retrieval_mode; > + uint32_t sat_mode; > + uint32_t enable_counters; > + uint32_t clear_counters; > + uint32_t enable_irq; > + uint32_t clear_irq; > + uint32_t trig_val_lo[TPDM_BC_MAX_COUNTERS]; > + uint32_t trig_val_hi[TPDM_BC_MAX_COUNTERS]; > + uint32_t enable_ganging; > + uint32_t overflow_val[TPDM_BC_MAX_OVERFLOW]; > + uint32_t msr[TPDM_BC_MAX_MSR]; > +}; > + > +struct tc_dataset { > + enum tpdm_mode capture_mode; > + enum tpdm_mode retrieval_mode; > + bool sat_mode; > + uint32_t enable_counters; > + uint32_t clear_counters; > + uint32_t enable_irq; > + uint32_t clear_irq; > + uint32_t trig_sel[TPDM_TC_MAX_TRIG]; > + uint32_t trig_val_lo[TPDM_TC_MAX_TRIG]; > + uint32_t trig_val_hi[TPDM_TC_MAX_TRIG]; > + uint32_t msr[TPDM_TC_MAX_MSR]; > +}; > + > +struct dsb_dataset { > + uint32_t mode; > + uint32_t edge_ctrl[TPDM_DSB_MAX_EDCR]; > + uint32_t edge_ctrl_mask[TPDM_DSB_MAX_EDCR / 2]; > + uint32_t patt_val[TPDM_DSB_MAX_PATT]; > + uint32_t patt_mask[TPDM_DSB_MAX_PATT]; > + bool patt_ts; > + bool patt_type; > + uint32_t trig_patt_val[TPDM_DSB_MAX_PATT]; > + uint32_t trig_patt_mask[TPDM_DSB_MAX_PATT]; > + bool trig_ts; > + bool trig_type; > + uint32_t select_val[TPDM_DSB_MAX_SELECT]; > + uint32_t msr[TPDM_DSB_MAX_MSR]; > +}; > + > +struct mcmb_dataset { > + uint8_t mcmb_trig_lane; > + uint8_t mcmb_lane_select; > +}; > + > +struct cmb_dataset { > + bool trace_mode; > + uint32_t cycle_acc; > + uint32_t patt_val[TPDM_CMB_PATT_CMP]; > + uint32_t patt_mask[TPDM_CMB_PATT_CMP]; > + bool patt_ts; > + uint32_t trig_patt_val[TPDM_CMB_PATT_CMP]; > + uint32_t trig_patt_mask[TPDM_CMB_PATT_CMP]; > + bool trig_ts; > + bool ts_all; > + uint32_t msr[TPDM_CMB_MAX_MSR]; > + uint8_t read_ctl_reg; > + struct mcmb_dataset *mcmb; > +}; > + > +DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); > + > +struct tpdm_drvdata { > + void __iomem *base; > + struct device *dev; > + struct coresight_device *csdev; > + int nr_tclk; > + struct clk **tclk; > + int nr_treg; > + struct regulator **treg; > + struct mutex lock; > + bool enable; > + bool clk_enable; > + DECLARE_BITMAP(datasets, TPDM_DATASETS); > + DECLARE_BITMAP(enable_ds, TPDM_DATASETS); > + enum tpdm_support_type tc_trig_type; > + enum tpdm_support_type bc_trig_type; > + enum tpdm_support_type bc_gang_type; > + uint32_t bc_counters_avail; > + uint32_t tc_counters_avail; > + struct gpr_dataset *gpr; > + struct bc_dataset *bc; > + struct tc_dataset *tc; > + struct dsb_dataset *dsb; > + struct cmb_dataset *cmb; > + int traceid; > + uint32_t version; > + bool msr_support; > + bool msr_fix_req; > + bool cmb_msr_skip; > +}; All of these should also be in a header file and properly documented. > + > +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata); This isn't needed. > + > +static void __tpdm_enable(struct tpdm_drvdata *drvdata) > +{ > + TPDM_UNLOCK(drvdata); > + > + if (drvdata->clk_enable) > + tpdm_writel(drvdata, 0x1, TPDM_CLK_CTRL); > + > + TPDM_LOCK(drvdata); > +} > + > +static int tpdm_enable(struct coresight_device *csdev, > + struct perf_event *event, u32 mode) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + int ret = 0; > + > + if (drvdata->enable) { This a race condition. > + dev_err(drvdata->dev, > + "TPDM setup already enabled,Skipping enablei\n"); Please remove this. > + return ret; Shouldn't this be return -EBUSY? > + } > + > + mutex_lock(&drvdata->lock); > + __tpdm_enable(drvdata); > + drvdata->enable = true; > + mutex_unlock(&drvdata->lock); > + > + dev_info(drvdata->dev, "TPDM tracing enabled\n"); > + return 0; > +} > + > +static void __tpdm_disable(struct tpdm_drvdata *drvdata) > +{ > + TPDM_UNLOCK(drvdata); > + > + if (drvdata->clk_enable) > + tpdm_writel(drvdata, 0x0, TPDM_CLK_CTRL); > + > + TPDM_LOCK(drvdata); > +} > + > +static void tpdm_disable(struct coresight_device *csdev, > + struct perf_event *event) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + if (!drvdata->enable) { > + dev_err(drvdata->dev, > + "TPDM setup already disabled, Skipping disable\n"); > + return; > + } > + mutex_lock(&drvdata->lock); > + __tpdm_disable(drvdata); > + drvdata->enable = false; > + mutex_unlock(&drvdata->lock); Same comments as above. > + > + dev_info(drvdata->dev, "TPDM tracing disabled\n"); > +} > + > +static int tpdm_trace_id(struct coresight_device *csdev) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + return drvdata->traceid; > +} > + > +static const struct coresight_ops_source tpdm_source_ops = { > + .trace_id = tpdm_trace_id, > + .enable = tpdm_enable, > + .disable = tpdm_disable, > +}; > + > +static const struct coresight_ops tpdm_cs_ops = { > + .source_ops = &tpdm_source_ops, > +}; > + > +static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata) > +{ > + if (test_bit(TPDM_DS_GPR, drvdata->datasets)) { > + drvdata->gpr = devm_kzalloc(drvdata->dev, sizeof(*drvdata->gpr), > + GFP_KERNEL); > + if (!drvdata->gpr) > + return -ENOMEM; > + } > + if (test_bit(TPDM_DS_BC, drvdata->datasets)) { > + drvdata->bc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->bc), > + GFP_KERNEL); > + if (!drvdata->bc) > + return -ENOMEM; > + } > + if (test_bit(TPDM_DS_TC, drvdata->datasets)) { > + drvdata->tc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->tc), > + GFP_KERNEL); > + if (!drvdata->tc) > + return -ENOMEM; > + } > + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) { > + drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb), > + GFP_KERNEL); > + if (!drvdata->dsb) > + return -ENOMEM; > + } > + if (test_bit(TPDM_DS_CMB, drvdata->datasets)) { > + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb), > + GFP_KERNEL); > + if (!drvdata->cmb) > + return -ENOMEM; > + } else if (test_bit(TPDM_DS_MCMB, drvdata->datasets)) { > + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb), > + GFP_KERNEL); > + if (!drvdata->cmb) > + return -ENOMEM; > + drvdata->cmb->mcmb = devm_kzalloc(drvdata->dev, > + sizeof(*drvdata->cmb->mcmb), > + GFP_KERNEL); > + if (!drvdata->cmb->mcmb) > + return -ENOMEM; How can I understand what the above does when: 1) There isn't a single line of comments. 2) I don't know the HW. 3) I don't have access to the documentation. > + } > + return 0; > +} > + > +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) > +{ > + if (test_bit(TPDM_DS_BC, drvdata->datasets)) > + drvdata->bc->retrieval_mode = TPDM_MODE_ATB; > + > + if (test_bit(TPDM_DS_TC, drvdata->datasets)) > + drvdata->tc->retrieval_mode = TPDM_MODE_ATB; > + > + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) { > + drvdata->dsb->trig_ts = true; > + drvdata->dsb->trig_type = false; > + } > + > + if (test_bit(TPDM_DS_CMB, drvdata->datasets) || > + test_bit(TPDM_DS_MCMB, drvdata->datasets)) > + drvdata->cmb->trig_ts = true; > +} > + > +static int tpdm_parse_of_data(struct tpdm_drvdata *drvdata) > +{ > + int i, ret; > + const char *tclk_name, *treg_name; > + struct device_node *node = drvdata->dev->of_node; > + > + drvdata->clk_enable = of_property_read_bool(node, "qcom,clk-enable"); > + drvdata->msr_fix_req = of_property_read_bool(node, "qcom,msr-fix-req"); > + drvdata->cmb_msr_skip = of_property_read_bool(node, > + "qcom,cmb-msr-skip"); > + > + drvdata->nr_tclk = of_property_count_strings(node, "qcom,tpdm-clks"); > + if (drvdata->nr_tclk > 0) { > + drvdata->tclk = devm_kzalloc(drvdata->dev, drvdata->nr_tclk * > + sizeof(*drvdata->tclk), > + GFP_KERNEL); > + if (!drvdata->tclk) > + return -ENOMEM; > + > + for (i = 0; i < drvdata->nr_tclk; i++) { > + ret = of_property_read_string_index(node, > + "qcom,tpdm-clks", i, &tclk_name); > + if (ret) > + return ret; > + > + drvdata->tclk[i] = devm_clk_get(drvdata->dev, > + tclk_name); > + if (IS_ERR(drvdata->tclk[i])) > + return PTR_ERR(drvdata->tclk[i]); > + } > + } > + > + drvdata->nr_treg = of_property_count_strings(node, "qcom,tpdm-regs"); > + if (drvdata->nr_treg > 0) { > + drvdata->treg = devm_kzalloc(drvdata->dev, drvdata->nr_treg * > + sizeof(*drvdata->treg), > + GFP_KERNEL); > + if (!drvdata->treg) > + return -ENOMEM; > + > + for (i = 0; i < drvdata->nr_treg; i++) { > + ret = of_property_read_string_index(node, > + "qcom,tpdm-regs", i, &treg_name); > + if (ret) > + return ret; > + > + drvdata->treg[i] = devm_regulator_get(drvdata->dev, > + treg_name); > + if (IS_ERR(drvdata->treg[i])) > + return PTR_ERR(drvdata->treg[i]); > + } > + } _None_ of the above are defined in the yaml file and/or part of the example that is shown there. Moreover they don't appear in patch 10/10 where TPDM and TPDA are supposed to be introduced. I will comment on patch 10/10 when I'm done with this one. > + > + return 0; > +} > + > +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > +{ > + int ret, i; > + uint32_t pidr, devid; > + struct device *dev = &adev->dev; > + struct coresight_platform_data *pdata; > + struct tpdm_drvdata *drvdata; > + struct coresight_desc desc = { 0 }; > + static int traceid = TPDM_TRACE_ID_START; > + uint32_t version; > + > + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + pdata = coresight_get_platform_data(dev); > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + adev->dev.platform_data = pdata; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + drvdata->dev = &adev->dev; > + dev_set_drvdata(dev, drvdata); > + > + drvdata->base = devm_ioremap_resource(dev, &adev->res); > + if (!drvdata->base) > + return -ENOMEM; > + > + mutex_init(&drvdata->lock); > + > + ret = tpdm_parse_of_data(drvdata); > + if (ret) { > + dev_err(drvdata->dev, "TPDM parse of data fail\n"); > + return -EINVAL; > + } > + > + desc.type = CORESIGHT_DEV_TYPE_SOURCE; > + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_PROC; Why is this of subtype CORESIGHT_DEV_SUBTYPE_SOURCE_PROC when TPDMs are not associated with a CPU? Here we should probably introduce something like CORESIGHT_DEV_SUBTYPE_SOURCE_SYS > + desc.ops = &tpdm_cs_ops; > + desc.pdata = adev->dev.platform_data; > + desc.dev = &adev->dev; > + drvdata->csdev = coresight_register(&desc); > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > + > + version = tpdm_readl(drvdata, CORESIGHT_PERIPHIDR2); > + drvdata->version = BMVAL(version, 4, 7); > + What is MSR support? This should be documented. Looking more closely at this patch, not a single line of documentation is provided. If you look at other drivers, you will find things to be quite different. The coresight subsystem has become very complex and documentation helps us, and anyone trying to contribute, understand what the code does. > + if (drvdata->version) > + drvdata->msr_support = true; > + > + pidr = tpdm_readl(drvdata, CORESIGHT_PERIPHIDR0); > + for (i = 0; i < TPDM_DATASETS; i++) { > + if (pidr & BIT(i)) { > + __set_bit(i, drvdata->datasets); > + __set_bit(i, drvdata->enable_ds); > + } > + } > + > + ret = tpdm_datasets_alloc(drvdata); > + if (ret) { > + coresight_unregister(drvdata->csdev); > + return ret; > + } > + > + tpdm_init_default_data(drvdata); > + > + devid = tpdm_readl(drvdata, CORESIGHT_DEVID); > + drvdata->tc_trig_type = BMVAL(devid, 27, 28); > + drvdata->bc_trig_type = BMVAL(devid, 25, 26); > + drvdata->bc_gang_type = BMVAL(devid, 23, 24); > + drvdata->bc_counters_avail = BMVAL(devid, 6, 10) + 1; > + drvdata->tc_counters_avail = BMVAL(devid, 4, 5) + 1; Shouldn't this be part of tpdm_init_default_data()? > + > + drvdata->traceid = traceid++; For this to work a new function needs to be introduced in coresight-pmu.h. Something like: static inline int coresight_get_system_trace_id(int id) { /* Start system IDs above the highest per CPU trace ID. */ return coresigth_get_trace_id(cpumask_last(cpu_possible_mask) + 1); } > + > + dev_dbg(drvdata->dev, "TPDM initialized\n"); Please remove this. > + > + if (boot_enable) > + coresight_enable(drvdata->csdev); > + > + pm_runtime_put(&adev->dev); > + > + return 0; > +} > + > +static struct amba_id tpdm_ids[] = { > + { > + .id = 0x001f0e00, > + .mask = 0x00ffff00, Any way we can use CS_AMBA_ID() here? > + .data = "TPDM", What is .data used for? > + }, > + { 0, 0}, > +}; > + > +static struct amba_driver tpdm_driver = { > + .drv = { > + .name = "coresight-tpdm", > + .owner = THIS_MODULE, > + .suppress_bind_attrs = true, > + }, > + .probe = tpdm_probe, > + .id_table = tpdm_ids, > +}; > + > +module_amba_driver(tpdm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); > -- > 2.17.1 >

3 years, 1 month

Re: [PATCH 04/10] Coresight: Enable BC and GPR for TPDM driver

by Mathieu Poirier

On Thu, Oct 21, 2021 at 03:38:50PM +0800, Tao Zhang wrote: > Enable GPR and Basic Counts(BC) for TPDM. Add GPR interface and But what is GPR? What does it stand for? Where is this documented? > basic control sysFS interface for TPDM. The GPR interface has RW > and RO fields for controlling external logic and mapping core > signals to an APB accessible address in the TPDM address map. > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 334 +++++++++++++++++++ > 1 file changed, 334 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 906776c859d6..c0a01979e42f 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -276,6 +276,93 @@ struct tpdm_drvdata { > > static void tpdm_init_default_data(struct tpdm_drvdata *drvdata); > > +static void __tpdm_enable_gpr(struct tpdm_drvdata *drvdata) There is no need for the double underscore at the beginning of the function name, please remove. The same goes for __tpdm_config_bc_msr() and __tpdm_enable_bc(). > +{ > + int i; > + > + for (i = 0; i < TPDM_GPR_REGS_MAX; i++) { > + if (!test_bit(i, drvdata->gpr->gpr_dirty)) > + continue; > + tpdm_writel(drvdata, drvdata->gpr->gp_regs[i], TPDM_GPR_CR(i)); > + } > +} > + > +static void __tpdm_config_bc_msr(struct tpdm_drvdata *drvdata) > +{ > + int i; > + > + if (!drvdata->msr_support) Shouldn't msr_support be part of bc_dataset? And why do we have a function for this when an if() condition would work just as well, as it is done in bc_trig_type below? > + return; > + > + for (i = 0; i < TPDM_BC_MAX_MSR; i++) > + tpdm_writel(drvdata, drvdata->bc->msr[i], TPDM_BC_MSR(i)); > +} > + > +static void __tpdm_enable_bc(struct tpdm_drvdata *drvdata) > +{ > + int i; > + uint32_t val; > + > + if (drvdata->bc->sat_mode) > + tpdm_writel(drvdata, drvdata->bc->sat_mode, > + TPDM_BC_SATROLL); > + else > + tpdm_writel(drvdata, 0x0, TPDM_BC_SATROLL); > + > + if (drvdata->bc->enable_counters) { > + tpdm_writel(drvdata, 0xFFFFFFFF, TPDM_BC_CNTENCLR); > + tpdm_writel(drvdata, drvdata->bc->enable_counters, > + TPDM_BC_CNTENSET); > + } > + if (drvdata->bc->clear_counters) > + tpdm_writel(drvdata, drvdata->bc->clear_counters, > + TPDM_BC_CNTENCLR); > + > + if (drvdata->bc->enable_irq) { > + tpdm_writel(drvdata, 0xFFFFFFFF, TPDM_BC_INTENCLR); > + tpdm_writel(drvdata, drvdata->bc->enable_irq, > + TPDM_BC_INTENSET); > + } > + if (drvdata->bc->clear_irq) > + tpdm_writel(drvdata, drvdata->bc->clear_irq, > + TPDM_BC_INTENCLR); > + > + if (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_FULL) { > + for (i = 0; i < drvdata->bc_counters_avail; i++) { Same here - shouldn't bc_trig_type and bc_counters_avail be part of bc_dataset? > + tpdm_writel(drvdata, drvdata->bc->trig_val_lo[i], > + TPDM_BC_TRIG_LO(i)); > + tpdm_writel(drvdata, drvdata->bc->trig_val_hi[i], > + TPDM_BC_TRIG_HI(i)); > + } > + } else if (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL) { > + tpdm_writel(drvdata, drvdata->bc->trig_val_lo[0], > + TPDM_BC_TRIG_LO(0)); > + tpdm_writel(drvdata, drvdata->bc->trig_val_hi[0], > + TPDM_BC_TRIG_HI(0)); > + } > + > + if (drvdata->bc->enable_ganging) > + tpdm_writel(drvdata, drvdata->bc->enable_ganging, TPDM_BC_GANG); > + > + for (i = 0; i < TPDM_BC_MAX_OVERFLOW; i++) > + tpdm_writel(drvdata, drvdata->bc->overflow_val[i], > + TPDM_BC_OVERFLOW(i)); > + > + __tpdm_config_bc_msr(drvdata); > + > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + if (drvdata->bc->retrieval_mode == TPDM_MODE_APB) > + val = val | BIT(2); > + else > + val = val & ~BIT(2); > + tpdm_writel(drvdata, val, TPDM_BC_CR); > + > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + /* Set the enable bit */ > + val = val | BIT(0); > + tpdm_writel(drvdata, val, TPDM_BC_CR); As a whole, this function is very hard to read and understand due to the lack of comments. > +} > + > static void __tpdm_enable(struct tpdm_drvdata *drvdata) > { > TPDM_UNLOCK(drvdata); > @@ -283,6 +370,12 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata) > if (drvdata->clk_enable) > tpdm_writel(drvdata, 0x1, TPDM_CLK_CTRL); > > + if (test_bit(TPDM_DS_GPR, drvdata->enable_ds)) > + __tpdm_enable_gpr(drvdata); > + > + if (test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + __tpdm_enable_bc(drvdata); > + > TPDM_LOCK(drvdata); > } > > @@ -307,10 +400,22 @@ static int tpdm_enable(struct coresight_device *csdev, > return 0; > } > > +static void __tpdm_disable_bc(struct tpdm_drvdata *drvdata) > +{ > + uint32_t config; > + > + config = tpdm_readl(drvdata, TPDM_BC_CR); > + config = config & ~BIT(0); > + tpdm_writel(drvdata, config, TPDM_BC_CR); > +} > + > static void __tpdm_disable(struct tpdm_drvdata *drvdata) > { > TPDM_UNLOCK(drvdata); > > + if (test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + __tpdm_disable_bc(drvdata); > + Shouldn't GPRs be disabled as well? If not why is it the case? A comment should be explaining what is going on. > if (drvdata->clk_enable) > tpdm_writel(drvdata, 0x0, TPDM_CLK_CTRL); > > @@ -352,6 +457,234 @@ static const struct coresight_ops tpdm_cs_ops = { > .source_ops = &tpdm_source_ops, > }; Everything related to sysfs should be in a patch on its own. > > +static ssize_t available_datasets_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) Indentation problem here and for the rest of the patch. > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + > + if (test_bit(TPDM_DS_IMPLDEF, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", > + "IMPLDEF"); > + > + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "DSB"); > + > + if (test_bit(TPDM_DS_CMB, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "CMB"); > + > + if (test_bit(TPDM_DS_TC, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "TC"); > + > + if (test_bit(TPDM_DS_BC, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "BC"); > + > + if (test_bit(TPDM_DS_GPR, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "GPR"); > + > + if (test_bit(TPDM_DS_MCMB, drvdata->datasets)) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "MCMB"); > + > + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > + return size; > +} > +static DEVICE_ATTR_RO(available_datasets); > + > +static ssize_t enable_datasets_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size; > + > + size = scnprintf(buf, PAGE_SIZE, "%*pb\n", TPDM_DATASETS, > + drvdata->enable_ds); > + > + if (PAGE_SIZE - size < 2) > + size = -EINVAL; TPDM_DATASETS is set to 32 - is this realistic? > + else > + size += scnprintf(buf + size, 2, "\n"); ...and what is going on here? > + return size; > +} > + > +static ssize_t enable_datasets_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + int i; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + > + mutex_lock(&drvdata->lock); > + if (drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + for (i = 0; i < TPDM_DATASETS; i++) { > + if (test_bit(i, drvdata->datasets) && (val & BIT(i))) > + __set_bit(i, drvdata->enable_ds); > + else > + __clear_bit(i, drvdata->enable_ds); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(enable_datasets); > + > +static ssize_t reset_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + int ret = 0; > + unsigned long val; > + struct mcmb_dataset *mcmb_temp = NULL; > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + ret = kstrtoul(buf, 10, &val); The coresight subsystem normally uses the hexadecimal base. > + if (ret) > + return ret; Shouldn't this be "if (!ret)" ? > + > + mutex_lock(&drvdata->lock); > + /* Reset all datasets to ZERO */ > + if (drvdata->gpr != NULL) > + memset(drvdata->gpr, 0, sizeof(struct gpr_dataset)); > + > + if (drvdata->bc != NULL) > + memset(drvdata->bc, 0, sizeof(struct bc_dataset)); > + > + if (drvdata->tc != NULL) > + memset(drvdata->tc, 0, sizeof(struct tc_dataset)); > + > + if (drvdata->dsb != NULL) > + memset(drvdata->dsb, 0, sizeof(struct dsb_dataset)); > + > + if (drvdata->cmb != NULL) { > + if (drvdata->cmb->mcmb != NULL) { > + mcmb_temp = drvdata->cmb->mcmb; > + memset(drvdata->cmb->mcmb, 0, > + sizeof(struct mcmb_dataset)); > + } > + > + memset(drvdata->cmb, 0, sizeof(struct cmb_dataset)); > + drvdata->cmb->mcmb = mcmb_temp; > + } > + /* Init the default data */ > + tpdm_init_default_data(drvdata); > + > + mutex_unlock(&drvdata->lock); > + > + /* Disable tpdm if enabled */ > + if (drvdata->enable) > + coresight_disable(drvdata->csdev); Why is this done out of the lock? > + > + return size; > +} > +static DEVICE_ATTR_WO(reset); > + > +static ssize_t integration_test_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + int i, ret = 0; > + unsigned long val; > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val != 1 && val != 2) > + return -EINVAL; > + > + if (!drvdata->enable) > + return -EINVAL; > + > + if (val == 1) > + val = ATBCNTRL_VAL_64; > + else > + val = ATBCNTRL_VAL_32; > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, 0x1, TPDM_ITCNTRL); > + > + for (i = 1; i < 5; i++) > + tpdm_writel(drvdata, val, TPDM_ITATBCNTRL); > + > + tpdm_writel(drvdata, 0, TPDM_ITCNTRL); > + TPDM_LOCK(drvdata); > + return size; > +} > +static DEVICE_ATTR_WO(integration_test); Integration test interface should be conditional to a compile time option. Have a look at what was done for CTIs. > + > +static ssize_t gp_regs_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_GPR, drvdata->datasets)) > + return -EPERM; return -EINVAL; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_GPR_REGS_MAX; i++) { > + if (!test_bit(i, drvdata->gpr->gpr_dirty)) > + continue; > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->gpr->gp_regs[i]); This should not be - the sysfs interface requires outputs of a single line. > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t gp_regs_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_GPR, drvdata->datasets) || > + index >= TPDM_GPR_REGS_MAX) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->gpr->gp_regs[index] = val; > + __set_bit(index, drvdata->gpr->gpr_dirty); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(gp_regs); > + > +static struct attribute *tpdm_attrs[] = { > + &dev_attr_available_datasets.attr, > + &dev_attr_enable_datasets.attr, > + &dev_attr_reset.attr, > + &dev_attr_integration_test.attr, > + &dev_attr_gp_regs.attr, > + NULL, > +}; All new sysfs interface need to be documented. See here: Documentation/ABI/testing/sysfs-bus-coresight-devices-xyz More comments to come... Thanks, Mathieu > + > +static struct attribute_group tpdm_attr_grp = { > + .attrs = tpdm_attrs, > +}; > +static const struct attribute_group *tpdm_attr_grps[] = { > + &tpdm_attr_grp, > + NULL, > +}; > + > static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata) > { > if (test_bit(TPDM_DS_GPR, drvdata->datasets)) { > @@ -513,6 +846,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > desc.ops = &tpdm_cs_ops; > desc.pdata = adev->dev.platform_data; > desc.dev = &adev->dev; > + desc.groups = tpdm_attr_grps; > drvdata->csdev = coresight_register(&desc); > if (IS_ERR(drvdata->csdev)) > return PTR_ERR(drvdata->csdev); > -- > 2.17.1 >

3 years, 1 month

Re: [PATCH 10/10] ARM: dts: msm: Add TPDA and TPDM support to DTS for RB5

by Mathieu Poirier

On Thu, Oct 21, 2021 at 03:38:56PM +0800, Tao Zhang wrote: > Add TPDA and TPDM support to DTS for RB5 board. This change is a > sample for validating. After applying this patch, the new TPDM and > TPDA nodes can be observed at the coresight devices path. TPDM and > TPDA hardware can be operated by commands. > > List the commands for validating this series patches as below. > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm0/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm0/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm0.bin > echo 0 > /sys/bus/coresight/devices/tpdm0/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm1/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm1/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm1/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm1.bin > echo 0 > /sys/bus/coresight/devices/tpdm1/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > echo 1 > /sys/bus/coresight/devices/tpdm2/enable_source > echo 1 > /sys/bus/coresight/devices/tpdm2/integration_test > echo 2 > /sys/bus/coresight/devices/tpdm2/integration_test > cat /dev/tmc_etf0 > /data/etf-tpdm2.bin > echo 0 > /sys/bus/coresight/devices/tpdm2/enable_source > echo 0 > /sys/bus/coresight/devices/tmc_etf0/enable_sink > > If the data from TPDMs can be obtained from the ETF, it means > that the TPDMs verification is successful. At the same time, > since TPDM0, TPDM1 and TPDM2 are all connected to the same > funnel "funnel@6c2d000" and output via different output ports, Where is "funnel@6c2d000" in this patch? > it also means that the following patches verification is > successful. > coresight: add support to enable more coresight paths > coresight: funnel: add support for multiple output ports > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 439 +++++++++++++++++++++++ > 1 file changed, 439 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index 8ac96f8e79d4..bcec8b181e11 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -222,6 +222,445 @@ > regulator-max-microvolt = <1800000>; > regulator-always-on; > }; > + > + stm@6002000 { > + compatible = "arm,coresight-stm", "arm,primecell"; > + reg = <0 0x06002000 0 0x1000>, > + <0 0x16280000 0 0x180000>; > + reg-names = "stm-base", "stm-stimulus-base"; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + stm_out: endpoint { > + remote-endpoint = > + <&funnel0_in7>; > + }; > + }; > + }; > + }; > + > + funnel@6041000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + reg = <0 0x06041000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + funnel0_out: endpoint { > + remote-endpoint = > + <&merge_funnel_in0>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@7 { > + reg = <7>; > + funnel0_in7: endpoint { > + remote-endpoint = <&stm_out>; > + }; > + }; > + }; > + }; > + > + funnel@6042000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + reg = <0 0x06042000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + funnel2_out: endpoint { > + remote-endpoint = > + <&merge_funnel_in2>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@4 { > + reg = <4>; > + funnel2_in5: endpoint { > + remote-endpoint = > + <&apss_merge_funnel_out>; > + }; > + }; > + }; > + }; > + > + funnel@6b04000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + arm,primecell-periphid = <0x000bb908>; > + > + reg = <0 0x6b04000 0 0x1000>; > + reg-names = "funnel-base"; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + merge_funnel_out: endpoint { > + remote-endpoint = > + <&etf_in>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@7 { > + reg = <7>; > + swao_funnel_in7: endpoint { > + slave-mode; > + remote-endpoint= > + <&merg_funnel_out>; > + }; > + }; > + }; > + > + }; > + > + funnel@6045000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + reg = <0 0x06045000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + merg_funnel_out: endpoint { > + remote-endpoint = <&swao_funnel_in7>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + merge_funnel_in0: endpoint { > + remote-endpoint = > + <&funnel0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + merge_funnel_in2: endpoint { > + remote-endpoint = > + <&funnel2_out>; > + }; > + }; > + }; > + }; > + > + etf@6b05000 { > + compatible = "arm,coresight-tmc", "arm,primecell"; > + reg = <0 0x06b05000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + in-ports { > + port { > + etf_in: endpoint { > + remote-endpoint = > + <&merge_funnel_out>; > + }; > + }; > + }; > + }; > + > + etm@7040000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07040000 0 0x1000>; > + > + cpu = <&CPU0>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm0_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in0>; > + }; > + }; > + }; > + }; > + > + etm@7140000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07140000 0 0x1000>; > + > + cpu = <&CPU1>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm1_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in1>; > + }; > + }; > + }; > + }; > + > + etm@7240000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07240000 0 0x1000>; > + > + cpu = <&CPU2>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm2_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in2>; > + }; > + }; > + }; > + }; > + > + etm@7340000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07340000 0 0x1000>; > + > + cpu = <&CPU3>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm3_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in3>; > + }; > + }; > + }; > + }; > + > + etm@7440000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07440000 0 0x1000>; > + > + cpu = <&CPU4>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm4_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in4>; > + }; > + }; > + }; > + }; > + > + etm@7540000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07540000 0 0x1000>; > + > + cpu = <&CPU5>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm5_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in5>; > + }; > + }; > + }; > + }; > + > + etm@7640000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07640000 0 0x1000>; > + > + cpu = <&CPU6>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm6_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in6>; > + }; > + }; > + }; > + }; > + > + etm@7740000 { > + compatible = "arm,coresight-etm4x", "arm,primecell"; > + reg = <0 0x07740000 0 0x1000>; > + > + cpu = <&CPU7>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + arm,coresight-loses-context-with-cpu; > + > + out-ports { > + port { > + etm7_out: endpoint { > + remote-endpoint = > + <&apss_funnel_in7>; > + }; > + }; > + }; > + }; > + > + funnel@7800000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + reg = <0 0x07800000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + apss_funnel_out: endpoint { > + remote-endpoint = > + <&apss_merge_funnel_in>; > + }; > + }; > + }; > + > + in-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + apss_funnel_in0: endpoint { > + remote-endpoint = > + <&etm0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + apss_funnel_in1: endpoint { > + remote-endpoint = > + <&etm1_out>; > + }; > + }; > + > + port@2 { > + reg = <2>; > + apss_funnel_in2: endpoint { > + remote-endpoint = > + <&etm2_out>; > + }; > + }; > + > + port@3 { > + reg = <3>; > + apss_funnel_in3: endpoint { > + remote-endpoint = > + <&etm3_out>; > + }; > + }; > + > + port@4 { > + reg = <4>; > + apss_funnel_in4: endpoint { > + remote-endpoint = > + <&etm4_out>; > + }; > + }; > + > + port@5 { > + reg = <5>; > + apss_funnel_in5: endpoint { > + remote-endpoint = > + <&etm5_out>; > + }; > + }; > + > + port@6 { > + reg = <6>; > + apss_funnel_in6: endpoint { > + remote-endpoint = > + <&etm6_out>; > + }; > + }; > + > + port@7 { > + reg = <7>; > + apss_funnel_in7: endpoint { > + remote-endpoint = > + <&etm7_out>; > + }; > + }; > + }; > + }; > + > + funnel@7810000 { > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell"; > + reg = <0 0x07810000 0 0x1000>; > + > + clocks = <&aoss_qmp>; > + clock-names = "apb_pclk"; > + > + out-ports { > + port { > + apss_merge_funnel_out: endpoint { > + remote-endpoint = > + <&funnel2_in5>; > + }; > + }; > + }; > + > + in-ports { > + port { > + apss_merge_funnel_in: endpoint { > + remote-endpoint = > + <&apss_funnel_out>; > + }; > + }; > + }; > + }; Which of the above introduces TPDM and TPDA devices? More comments to come tomorrow. Thanks, Mathieu > }; > > &adsp { > -- > 2.17.1 >

3 years, 1 month

Re: [PATCH 4/5] perf arm-spe: Implement find_snapshot callback

by German Gomez

Hi Leo, Would you be ok with the current patch the way it is? In case it's of any help, I'm sharing the testing steps that James and I went through when testing this internally, if you want to add to it - Test that only a portion of the buffer is saved until there is a wraparound $ ./perf record -vvv -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 & while true; do sleep 0.2; killall -s USR2 perf; done - Test snapshot mode in CPU mode $ sudo ./perf record -vvv -C 0 -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 & - Test that auxtrace buffers correspond to an aux record - Test snapshot default sizes in sudo and user modes - Test small snapshot size $ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 & If there are any concerns with the patches, please let me know and I will try to address them. Thanks, German On 13/10/2021 08:51, Will Deacon wrote: > On Wed, Oct 13, 2021 at 08:39:16AM +0800, Leo Yan wrote: >> On Mon, Oct 11, 2021 at 04:55:37PM +0100, German Gomez wrote: >>> On 06/10/2021 10:51, Leo Yan wrote: >>>> On Wed, Oct 06, 2021 at 10:35:20AM +0100, German Gomez wrote: >>>> >>>> [...] >>>> >>>>>> So simply say, I think the head pointer monotonically increasing is >>>>>> the right thing to do in Arm SPE driver. >>>>> I will talk to James about how we can proceed on this. >>>> Thanks! >>> I took this offline with James and, though it looks possible to patch >>> the SPE driver to have a monotonically increasing head pointer in order >>> to simplify the handling in the perf tool, it could be a breaking change >>> for users of the perf_event_open syscall that currently rely on the way >>> it works now. >> Here I cannot create the connection between AUX head pointer and the >> breakage of calling perf_event_open(). >> >> Could you elaborate what's the reason the monotonical increasing head >> pointer will lead to the breakage for perf_event_open()? > It's a user-visible change in behaviour, isn't it? Therefore we risk > breaking applications that rely on the current behaviour if we change it > unconditionally. > > Given that the driver has always worked like this and it doesn't sound like > it's the end of the world to deal with it in userspace (after all, it's > aligned with intel-pt), then I don't think we should change it. > > Will

3 years, 1 month

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight November 2021