CoreSight

coresight@lists.linaro.org

4 participants
2607 discussions

Re: [PATCH] perf cs-etm: use swap() to make code cleaner

by Leo Yan

Hi Yihao, On Thu, Nov 11, 2021 at 06:33:44PM -0800, Yihao Han wrote: > Use the macro 'swap()' defined in 'include/linux/minmax.h' to avoid > opencoding it. The header 'include/linux/minmax.h' is used for kernel but it's not provided for user space program? > Signed-off-by: Yihao Han <hanyihao(a)vivo.com> > --- > tools/perf/util/cs-etm.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index f323adb1af85..aaa3ec725002 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -408,17 +408,13 @@ struct cs_etm_packet_queue > static void cs_etm__packet_swap(struct cs_etm_auxtrace *etm, > struct cs_etm_traceid_queue *tidq) > { > - struct cs_etm_packet *tmp; > - > if (etm->sample_branches || etm->synth_opts.last_branch || > etm->sample_instructions) { > /* > * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for > * the next incoming packet. > */ > - tmp = tidq->packet; > - tidq->packet = tidq->prev_packet; > - tidq->prev_packet = tmp; > + swap(tidq->packet, tidq->prev_packet); NAK - This patch will introduce compilation error: util/cs-etm.c: In function ‘cs_etm__packet_swap’: util/cs-etm.c:417:3: error: implicit declaration of function ‘swap’; did you mean ‘swab’? [-Werror=implicit-function-declaration] swap(tidq->packet, tidq->prev_packet); ^~~~ swab Thanks, Leo > } > } > > -- > 2.17.1 >

4 years, 2 months

Re: [PATCH v2] coresight: Use devm_bitmap_zalloc when applicable

by Mathieu Poirier

Hi Christophe, On Wed, 3 Nov 2021 at 12:39, Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> wrote: > > 'drvdata->chs.guaranteed' is a bitmap. So use 'devm_bitmap_kzalloc()' to > simplify code, improve the semantic and avoid some open-coded arithmetic > in allocator arguments. > > Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> > --- > v1 --> v2: remove the 'guaranteed' variable to be even less verbose > --- > drivers/hwtracing/coresight/coresight-stm.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index 58062a5a8238..bb14a3a8a921 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -856,13 +856,11 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) > { > int ret; > void __iomem *base; > - unsigned long *guaranteed; > struct device *dev = &adev->dev; > struct coresight_platform_data *pdata = NULL; > struct stm_drvdata *drvdata; > struct resource *res = &adev->res; > struct resource ch_res; > - size_t bitmap_size; > struct coresight_desc desc = { 0 }; > > desc.name = coresight_alloc_device_name(&stm_devs, dev); > @@ -904,12 +902,10 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) > else > drvdata->numsp = stm_num_stimulus_port(drvdata); > > - bitmap_size = BITS_TO_LONGS(drvdata->numsp) * sizeof(long); > - > - guaranteed = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); > - if (!guaranteed) > + drvdata->chs.guaranteed = devm_bitmap_zalloc(dev, drvdata->numsp, > + GFP_KERNEL); > + if (!drvdata->chs.guaranteed) > return -ENOMEM; > - drvdata->chs.guaranteed = guaranteed; I will pick up this patch when -rc1 gets released. Thanks, Mathieu > > spin_lock_init(&drvdata->spinlock); > > -- > 2.30.2 >

4 years, 2 months

[PATCH v2 0/2] arm-spe/cs-etm: Print size using consistent format

by German Gomez

Sent as a splitted re-roll of patchset [1] to keep the patches more semantically grouped. The patches themselves have been tested and reviewed in the previous submission already. Changes since v1: - Sent as a separate patchset. - Added acme to the recipients list as I forgot to include him in [1]. [1] https://lore.kernel.org/all/20210916154635.1525-1-german.gomez@arm.com/ Andrew Kilroy (2): perf cs-etm: Print size using consistent format perf arm-spe: Print size using consistent format tools/perf/util/arm-spe.c | 2 +- tools/perf/util/cs-etm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.25.1

4 years, 2 months

[PATCH 1/5] perf cs-etm: Print size using consistent format

by German Gomez

From: Andrew Kilroy <andrew.kilroy(a)arm.com> Since the size is already printed earlier in hex, print the same data using the same format, in hex. Reviewed-by: James Clark <james.clark(a)arm.com> Signed-off-by: Andrew Kilroy <andrew.kilroy(a)arm.com> Signed-off-by: German Gomez <german.gomez(a)arm.com> --- tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..4f672f7d008c 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -537,7 +537,7 @@ static void cs_etm__dump_event(struct cs_etm_queue *etmq, fprintf(stdout, "\n"); color_fprintf(stdout, color, - ". ... CoreSight %s Trace data: size %zu bytes\n", + ". ... CoreSight %s Trace data: size %#zx bytes\n", cs_etm_decoder__get_name(etmq->decoder), buffer->size); do { -- 2.17.1

4 years, 2 months

Re: [PATCH v2] drivers/coresight: ultrasoc: Add System Memory Buffer driver

by liuqi (BA)

On 2021/11/5 9:36, Randy Dunlap wrote: > On 11/4/21 6:20 PM, Qi Liu wrote: >> diff --git a/drivers/hwtracing/coresight/ultrasoc/Kconfig >> b/drivers/hwtracing/coresight/ultrasoc/Kconfig >> new file mode 100644 >> index 000000000000..c18c25bf9df3 >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc/Kconfig >> @@ -0,0 +1,12 @@ >> +# SPDX-License-Identifier: MIT >> +# >> +# ultrasoc configuration >> +# >> +config ULTRASOC_SMB >> + tristate "Ultrasoc system memory buffer drivers" >> + depends on ARM64 && CORESIGHT > > The "depends" line should be indented with a tab instead of spaces... > ah, yes, my editor might be set incorrectly, thanks for the reminder, I'll correct it next time. Qi >> + help >> + This enables support for the Ultrasoc system memory buffer >> + driver that is responsible for receiving the trace data from >> + Coresight ETM devices and storing them to a system buffer >> + respectively. > >

4 years, 2 months

[GIT PULL] Coresight fix for v5.16

by Mathieu Poirier

The following changes since commit 561ced0bb90a4be298b7db5fb54f29731d74a3f6: arm64: errata: Enable TRBE workaround for write to out-of-range address (2021-10-27 11:46:06 -0600) are available in the Git repository at: git@gitolite.kernel.org:pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-fixes-v5.16 for you to fetch changes up to 20b02fe36530a1e48dde73c80b882ae276346ea3: arm64: cpufeature: Export this_cpu_has_cap helper (2021-11-04 10:46:21 -0600) --------------------------------------------------------------- coresight: Fix for v5.16 This fix enables to compile the TRBE driver as a module by exporting function this_cpu_has_cap(). Signed-off-by: Mathieu Poirier <mathieu.poirier(a)linaro.org> ---------------------------------------------------------------- Arnd Bergmann (1): arm64: cpufeature: Export this_cpu_has_cap helper arch/arm64/kernel/cpufeature.c | 1 + 1 file changed, 1 insertion(+)

4 years, 2 months

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.

4 years, 2 months

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 >

4 years, 2 months

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

4 years, 2 months

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

4 years, 2 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight