CoreSight

coresight@lists.linaro.org

7 participants
2442 discussions

Re: [PATCH v2 0/5] coresight: ctcu: Enable byte-cntr function for TMC ETR

by Mike Leach

Hi, I can see that this patchset has fixed some of the issues raised from v1 - using the existing file handle for read, and stopping the ETR to read the RWP. However the fundamental problem that it is still attempting to read memory without stopping the ETR has not been addressed. As mentioned in mine and Suzuki's comments for v1, this means:- 1) you cannot guarantee that the buffer has not wrapped when reading data back, which will corrupt the trace decode process. 2) The DMA buffers are not being synchronized, so the PE could be reading stale data rather than the new data written by the ETR. Regards Mike On Thu, 10 Apr 2025 at 02:33, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote: > > The byte-cntr function provided by the CTCU device is used to transfer data > from the ETR buffer to the userspace. An interrupt is tiggered if the data > size exceeds the threshold set in the BYTECNTRVAL register. The interrupt > handler counts the number of triggered interruptions and the read function > will read the data from the ETR buffer if the IRQ count is greater than 0. > The read work will be conducted ASAP after the byte-cntr is started. > Each successful read process will decrement the IRQ count by 1. > > The byte cntr function will start when the device node is opened for reading, > and the IRQ count will reset when the byte cntr function has stopped. When > the file node is opened, the w_offset of the ETR buffer will be read and > stored in byte_cntr_data, serving as the original r_offset (indicating > where reading starts) for the byte counter function. > > The work queue for the read operation will wake up once when ETR is stopped, > ensuring that the remaining data in the ETR buffer has been flushed based on > the w_offset read at the time of stopping. > > The byte-cntr read work has integrated with the file node tmc_etr, e.g. > /dev/tmc_etr0 > /dev/tmc_etr1 > > There are two scenarios for the ETR file nodes with byte-cntr function: > 1. BYTECNTRVAL register has configured -> byte-cntr read > 2. BYTECNTRVAL register is disabled -> original behavior, flush the etr_buf > > We still can flush the etr buffer once after the byte-cntr function has > triggered. > 1. Enable byte-cntr > 2. Byte-cntr read > 3. Disable byte-cntr > 4. Flush etr buffer > > Since the ETR operates in circular buffer mode, we cannot fully guarantee > that no overwrites occur when the byte-cntr read function reads the data. > The read function will read the data ASAP when the interrupt is > triggered and we should not configure a threshold greater than the > buffer size of the ETR buffer. > > The following shell commands write threshold to BYTECNTRVAL registers. > > Only enable byte-cntr for ETR0: > echo 0x10000 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Enable byte-cntr for both ETR0 and ETR1(support both hex and decimal values): > echo 0x10000 4096 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Setting the BYTECNTRVAL registers to 0 disables the byte-cntr function. > Disable byte-cntr for ETR0: > echo 0 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Disable byte-cntr for both ETR0 and ETR1: > echo 0 0 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > There is a minimum threshold to prevent generating too many interrupts. > The minimum threshold is 4096 bytes. The write process will fail if user try > to set the BYTECNTRVAL registers to a value less than 4096 bytes(except > for 0). > > Way to enable and start byte-cntr for ETR0: > echo 0x10000 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink > echo 1 > /sys/bus/coresight/devices/etm0/enable_source > cat /dev/tmc_etr0 > > Testing case has conducted for the byte-cntr read work: > 1. Setting the buffer_size of the ETR as large as possile, here is for ETR0 > echo 0x1000000 > /sys/bus/coresight/devices/tmc_etr0/buffer_size > 2. Setting the threshold for the ETR0 to 0x10000 > echo 0x10000 > /sys/bus/coresight/devices/ctcu0/byte_cntr_val > 3. Enable ETR0 > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink > 4. Enable ETM0 as source and enable byte-cntr to read data > echo 1 > /sys/bus/coresight/devices/etm0/enable_source; > cat /dev/tmc_etr0 > /tmp/file_byte_cntr.bin & > 5. Disable ETM0 > echo 0 > /sys/bus/coresight/devices/etm0/enable_source > 6. Disable byte-cntr and flush the etr buffer > echo 0 > /sys/bus/coresight/devices/ctcu0/byte_cntr_val; > cat /dev/tmc_etr0 > /tmp/file_etr0.bin > ls -l /tmp > > -rw-r--r-- 1 root root 12628960 Apr 28 17:44 file_byte_cntr.bin > -rw-r--r-- 1 root root 12669296 Apr 28 17:45 file_etr0.bin > > 7. Deal with the file_etr0.bin with following command: > dd if=/tmp/file_etr0.bin of=/tmp/file_etr0_aligned.bin bs=1 > count=12628960 skip=40336 > ls -l /tmp > > -rw-r--r-- 1 root root 12628960 Apr 28 17:44 file_byte_cntr.bin > -rw-r--r-- 1 root root 12669296 Apr 28 17:45 file_etr0.bin > -rw-r--r-- 1 root root 12628960 Apr 28 17:49 file_etr0_aligned.bin > > 8. Compared file_byte_cntr.bin with file_etr0_aligned.bin and identified > they are competely same. > diff file_byte_cntr.bin file_etr0_aligned.bin > > ======================= > Changes in V2: > 1. Removed the independent file node /dev/byte_cntr. > 2. Integrated the byte-cntr's file operations with current ETR file > node. > 3. Optimized the driver code of the CTCU that associated with byte-cntr. > 4. Add kernel document for the export API tmc_etr_get_rwp_offset. > 5. Optimized the way to read the rwp_offset according to Mike's > suggestion. > 6. Removed the dependency of the dts patch. > Link to V1 - https://lore.kernel.org/all/20250310090407.2069489-1-quic_jiegan@quicinc.co… > > Jie Gan (5): > coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer > dt-bindings: arm: Add an interrupt property for Coresight CTCU > coresight: ctcu: Enable byte-cntr for TMC ETR devices > coresight: tmc: add functions for byte-cntr operation > arm64: dts: qcom: sa8775p: Add interrupts to CTCU device > > .../bindings/arm/qcom,coresight-ctcu.yaml | 17 ++ > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 5 + > drivers/hwtracing/coresight/Makefile | 2 +- > .../coresight/coresight-ctcu-byte-cntr.c | 119 ++++++++++++ > .../hwtracing/coresight/coresight-ctcu-core.c | 88 ++++++++- > drivers/hwtracing/coresight/coresight-ctcu.h | 49 ++++- > .../hwtracing/coresight/coresight-tmc-core.c | 29 ++- > .../hwtracing/coresight/coresight-tmc-etr.c | 175 ++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tmc.h | 10 +- > 9 files changed, 483 insertions(+), 11 deletions(-) > create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c > > -- > 2.34.1 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

Re: [PATCH v3 4/7] coresight-tgu: Add TGU decode support

by Mike Leach

Hi, On Thu, 10 Apr 2025 at 08:46, songchai <quic_songchai(a)quicinc.com> wrote: > > > On 3/7/2025 6:58 PM, Mike Leach wrote: > > Hi, > > On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote: > > From: Songwei Chai <quic_songchai(a)quicinc.com> > > Decoding is when all the potential pieces for creating a trigger > are brought together for a given step. Example - there may be a > counter keeping track of some occurrences and a priority-group that > is being used to detect a pattern on the sense inputs. These 2 > inputs to condition_decode must be programmed, for a given step, > to establish the condition for the trigger, or movement to another > step. > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > Signed-off-by: songchai <quic_songchai(a)quicinc.com> > --- > .../testing/sysfs-bus-coresight-devices-tgu | 7 ++ > drivers/hwtracing/coresight/coresight-tgu.c | 113 ++++++++++++++++-- > drivers/hwtracing/coresight/coresight-tgu.h | 29 ++++- > 3 files changed, 136 insertions(+), 13 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > index af7332153833..dd6cc1184d52 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > @@ -14,3 +14,10 @@ KernelVersion 6.15 > Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > Description: > (RW) Set/Get the sensed siganal with specific step and priority for TGU. > + > +What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_condition_decode/reg[0:3] > +Date: February 2025 > +KernelVersion 6.15 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > +Description: > + (RW) Set/Get the decode mode with specific step for TGU. > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c > index f28761619ebe..5eebf5eecbbb 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.c > +++ b/drivers/hwtracing/coresight/coresight-tgu.c > @@ -22,9 +22,21 @@ static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index, > { > int ret = -EINVAL; > > - ret = operation_index * (drvdata->max_step) * > - (drvdata->max_reg) + step_index * (drvdata->max_reg) > - + reg_index; > + switch (operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = operation_index * (drvdata->max_step) * > + (drvdata->max_reg) + step_index * (drvdata->max_reg) > + + reg_index; > + break; > + case TGU_CONDITION_DECODE: > + ret = step_index * (drvdata->max_condition_decode) + reg_index; > + break; > + default: > + break; > + } > > return ret; > } > @@ -36,10 +48,23 @@ static ssize_t tgu_dataset_show(struct device *dev, > struct tgu_attribute *tgu_attr = > container_of(attr, struct tgu_attribute, attr); > > - return sysfs_emit(buf, "0x%x\n", > - drvdata->value_table->priority[calculate_array_location( > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->priority[calculate_array_location( > > calculate_array_location() can return -EINVAL - you could be > referencing array value drvdata->value_table->priority[-EINVAL] here. > Test the return before referencing the array. > > Same for all following occurrences. > > Both tgu_dataset_store/show and calculate_array_location use switch cases to evaluate tgu_attr->operation_index. > > Therefore, when a case in tgu_dataset_store/show satisfies tgu_attr->operation_index, calculate_array_location > > will not return -EINVAL. This prevents the occurrence of -EINVAL within the array. If the function can never return -EINVAL, then do not use is at all in the function - it is unneeded. It does not matter that you believe that the current implementation can never produce an incorrect response - if you write a function that can return -EINVAL, this must be checked for - do not assume that future changes will retain the apparent "safety". Regards Mike > > drvdata, tgu_attr->step_index, > tgu_attr->operation_index, tgu_attr->reg_num)]); > + case TGU_CONDITION_DECODE: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->condition_decode[calculate_array_location( > + drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)]); > + > > missing default - did this code compile without warnings? > > Will correct it in next version. > > + } > + return -EINVAL; > > } > > @@ -58,11 +83,25 @@ static ssize_t tgu_dataset_store(struct device *dev, > return ret; > > guard(spinlock)(&tgu_drvdata->spinlock); > - tgu_drvdata->value_table->priority[calculate_array_location( > - tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > - tgu_attr->reg_num)] = val; > - ret = size; > - > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + tgu_drvdata->value_table->priority[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + case TGU_CONDITION_DECODE: > + tgu_drvdata->value_table->condition_decode[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + default: > + break; > + } > return ret; > } > > @@ -79,8 +118,23 @@ static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *attr, > container_of(dev_attr, struct tgu_attribute, attr); > > if (tgu_attr->step_index < drvdata->max_step) { > - ret = (tgu_attr->reg_num < drvdata->max_reg) ? > - attr->mode : 0; > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = (tgu_attr->reg_num < drvdata->max_reg) ? > + attr->mode : 0; > + break; > + case TGU_CONDITION_DECODE: > + ret = (tgu_attr->reg_num < > + drvdata->max_condition_decode) ? > + attr->mode : 0; > + break; > + default: > + break; > + } > + > return ret; > } > return SYSFS_GROUP_INVISIBLE; > @@ -103,6 +157,17 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata) > } > } > > + for (i = 0; i < drvdata->max_step; i++) { > + for (j = 0; j < drvdata->max_condition_decode; j++) { > + tgu_writel(drvdata, > + drvdata->value_table > + ->condition_decode[calculate_array_location( > + drvdata, i, > + TGU_CONDITION_DECODE, j)], > + CONDITION_DECODE_STEP(i, j)); > + } > + } > + > /* Enable TGU to program the triggers */ > tgu_writel(drvdata, 1, TGU_CONTROL); > CS_LOCK(drvdata->base); > @@ -245,6 +310,14 @@ static const struct attribute_group *tgu_attr_groups[] = { > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(0), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(1), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(2), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(4), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7), > NULL, > }; > > @@ -289,6 +362,13 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (ret) > return -EINVAL; > > + ret = of_property_read_u32(adev->dev.of_node, "tgu-conditions", > + &drvdata->max_condition); > + if (ret) > + return -EINVAL; > + > + drvdata->max_condition_decode = drvdata->max_condition; > + > drvdata->value_table = > devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL); > if (!drvdata->value_table) > @@ -303,6 +383,15 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (!drvdata->value_table->priority) > return -ENOMEM; > > + drvdata->value_table->condition_decode = devm_kzalloc( > + dev, > + drvdata->max_condition_decode * drvdata->max_step * > + sizeof(*(drvdata->value_table->condition_decode)), > + GFP_KERNEL); > + > + if (!drvdata->value_table->condition_decode) > + return -ENOMEM; > + > drvdata->enable = false; > desc.type = CORESIGHT_DEV_TYPE_HELPER; > desc.pdata = adev->dev.platform_data; > diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h > index 6e5d465117df..c2a9ce38b44f 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.h > +++ b/drivers/hwtracing/coresight/coresight-tgu.h > @@ -46,6 +46,9 @@ > #define PRIORITY_REG_STEP(step, priority, reg)\ > (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step) > > +#define CONDITION_DECODE_STEP(step, decode) \ > + (0x0050 + 0x4 * decode + 0x1D8 * step) > + > > use #define constants with explanations of what they are rather than > arbitrary magic numbers. > > Done. > > #define tgu_dataset_ro(name, step_index, type, reg_num) \ > (&((struct tgu_attribute[]){ { \ > __ATTR(name, 0444, tgu_dataset_show, NULL), \ > @@ -66,6 +69,9 @@ > tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \ > reg_num) > > +#define STEP_DECODE(step_index, reg_num) \ > + tgu_dataset_rw(reg##reg_num, step_index, TGU_CONDITION_DECODE, reg_num) > + > #define STEP_PRIORITY_LIST(step_index, priority) \ > {STEP_PRIORITY(step_index, 0, priority), \ > STEP_PRIORITY(step_index, 1, priority), \ > @@ -88,6 +94,14 @@ > NULL \ > } > > +#define STEP_DECODE_LIST(n) \ > + {STEP_DECODE(n, 0), \ > + STEP_DECODE(n, 1), \ > + STEP_DECODE(n, 2), \ > + STEP_DECODE(n, 3), \ > + NULL \ > + } > + > #define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\ > (&(const struct attribute_group){\ > .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\ > @@ -95,11 +109,19 @@ > .name = "step" #step "_priority" #priority \ > }) > > +#define CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(step)\ > + (&(const struct attribute_group){\ > + .attrs = (struct attribute*[])STEP_DECODE_LIST(step),\ > + .is_visible = tgu_node_visible,\ > + .name = "step" #step "_condition_decode" \ > + }) > + > enum operation_index { > TGU_PRIORITY0, > TGU_PRIORITY1, > TGU_PRIORITY2, > - TGU_PRIORITY3 > + TGU_PRIORITY3, > + TGU_CONDITION_DECODE > > }; > > @@ -115,6 +137,7 @@ struct tgu_attribute { > > struct value_table { > unsigned int *priority; > + unsigned int *condition_decode; > }; > > /** > @@ -127,6 +150,8 @@ struct value_table { > * @value_table: Store given value based on relevant parameters. > * @max_reg: Maximum number of registers > * @max_step: Maximum step size > + * @max_condition: Maximum number of condition > + * @max_condition_decode: Maximum number of condition_decode > * > * This structure defines the data associated with a TGU device, including its base > * address, device pointers, clock, spinlock for synchronization, trigger data pointers, > @@ -141,6 +166,8 @@ struct tgu_drvdata { > struct value_table *value_table; > int max_reg; > int max_step; > + int max_condition; > + int max_condition_decode; > }; > > #endif > > Regards > > Mike > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

Re: [PATCH] [Patch v3]Add ETE devarch condition in etm4_init_iomem_access

by Mike Leach

Hi, Please stop using the unusual title format of [PATCH}[Patch v3] - use the git command line options to ensure the format is e.g. [PATCH v3] What is the reason that your device cannot use the system instructions to access the ETE? Using a memory interface, if implemented, is only recommended for external debuggers, or on systems where the implementation of system register access is not working. On Thu, 10 Apr 2025 at 10:52, yiru zhang <yiru.zhang(a)mediatek.com> wrote: > > Due to ETE supported, so add ETE devarch condition in etm4_init_iomem_access. > > Signed-off-by: yiru zhang <yiru.zhang(a)mediatek.com> > Reported-by: kernel test robot <lkp(a)intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202504101759.7Ls0Uy4o-lkp@intel.com/ > > v1->v2: use switch case way > v2->v3: clean build warning > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 2b8f10463840..4002a2823fd0 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -1135,11 +1135,15 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata, > * with MMIO. But we cannot touch the OSLK until we are > * sure this is an ETM. So rely only on the TRCDEVARCH. > */ > - if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) { > - pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n"); > + switch (devarch & ETM_DEVARCH_ID_MASK) { > + case ETM_DEVARCH_ETMv4x_ARCH: > + case ETM_DEVARCH_ETE_ARCH: > + break; > + default: > + pr_warn_once("Unknown ETM architecture: 0x%lx\n", > + devarch & ETM_DEVARCH_ID_MASK); > return false; > } > - > drvdata->arch = etm_devarch_to_arch(devarch); > *csa = CSDEV_ACCESS_IOMEM(drvdata->base); > return true; > -- > 2.46.0 > Otherwise - Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

[PATCH v2 0/5] coresight: Add Coresight Trace NOC driver

by Yuanfang Zhang

The Trace NoC is an integration hierarchy which is a replacement of Dragonlink configuration. It brings together debug component like TPDA, funnel and interconnect Trace Noc which collects trace from subsystems and transfers to QDSS sink. Compared to DL, it has the following advantages: 1. Reduce wires between subsystems. 2. Continue cleaning the infrastructure. 3. Reduce Data overhead by transporting raw data from source to target. +--------------+ +-------------+ | SDCC5 TPDM | | SDCC5 TPDM | +--------------+ +-------------+ | | | | +----------|-------------------+ | | v | | | +----v----+ Dragon Link | v | |DLNT TPDA| North | +----------------------+ | +---------+ | | TRACE NOC AG | | | | | | | v-------------+ | +----------------------+ | | | | | +------v-----+ | | | | DLNT Funnel| | | | +------------+ | | | | | | +-------------------|----------+ | <-----+ | | | | | v v +----------------+ +---------------+ | QDSS | | QDSS | +----------------+ +---------------+ Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com> --- --- Changes in v2: 1. Modified the format of DT binging file. 2. Fix compile warnings. - Link to v1: https://lore.kernel.org/r/20250221-trace-noc-driver-v1-0-0a23fc643217@quici… --- Yuanfang Zhang (5): dt-bindings: arm: Add Coresight device Trace NOC definition coresight: add coresight Trace NOC driver coresight-tnoc: add nodes to configure flush coresight-tnoc: add node to configure flag type coresight-tnoc: add nodes to configure freq packet .../bindings/arm/qcom,coresight-tnoc.yaml | 116 ++++++ drivers/hwtracing/coresight/Kconfig | 13 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-tnoc.c | 400 +++++++++++++++++++++ drivers/hwtracing/coresight/coresight-tnoc.h | 57 +++ 5 files changed, 587 insertions(+) --- base-commit: 92514ef226f511f2ca1fb1b8752966097518edc0 change-id: 20250212-trace-noc-driver-9e75d78114fa Best regards, -- Yuanfang Zhang <quic_yuanfang(a)quicinc.com>

4 months, 2 weeks

Re: [PATCH] [Patch v1]Add ETE devarch condition in etm4_init_iomem_access

by Suzuki K Poulose

Hi Yiru On 08/04/2025 08:36, yiru zhang wrote: > Due to ETE supported, so add ETE devarch condition in etm4_init_iomem_access. Is there a reason why you cannot use the "system instructions to access the ETE" ? The patch as such is fine by me (withe some minor styling nits). But, we do not recommend using the MMIO for ETE, when it can be accessed directly by sysreg. FWIW, if you remove the "mmio base" address from the DT/ACPI, it should automatically use the system instructions for ETE. > Signed-off-by: yiru zhang <yiru.zhang(a)mediatek.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 2b8f10463840..971b9f0fe5e4 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -1135,8 +1135,9 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata, > * with MMIO. But we cannot touch the OSLK until we are > * sure this is an ETM. So rely only on the TRCDEVARCH. > */ > - if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) { > - pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n"); > + if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH \ > + && (devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETE_ARCH) { We could use a switch case ? switch (devarch & ETM_DEVARCH_ID_MASK) { case ETM_DEVARCH_ETMv4x_ARCH: case ETM_DEVARCH_ETE_ARCH: break; default: pr_warn_once("Unknown ETM architecture: %x\n", devarch & ETM_DEVARCH_ID_MASK); return false; } Suzuki > + pr_warn_once("TRCDEVARCH doesn't match ETMv4&ETE architecture\n"); > return false; > } >

4 months, 2 weeks

Re: [PATCH v2 1/3] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by Suzuki K Poulose

On 03/04/2025 18:53, Yabin Cui wrote: > On Wed, Apr 2, 2025 at 6:01 AM Leo Yan <leo.yan(a)arm.com> wrote: >> >> Hi Yabin, >> >> On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote: >> >> [...] >> >>>> @@ -486,12 +491,17 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) >>>> >>>> static int catu_disable(struct coresight_device *csdev, void *__unused) >>>> { >>>> - int rc; >>>> + int rc = 0; >>>> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); >>>> + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); >>>> >>>> - CS_UNLOCK(catu_drvdata->base); >>>> - rc = catu_disable_hw(catu_drvdata); >>>> - CS_LOCK(catu_drvdata->base); >>>> + if (--csdev->refcnt == 0) { >>>> + CS_UNLOCK(catu_drvdata->base); >>>> + rc = catu_disable_hw(catu_drvdata); >>>> + CS_LOCK(catu_drvdata->base); >>>> + } else { >>>> + rc = -EBUSY; >> >> This is not an error if the decremented reference counter is not zero. >> It should return 0. Otherwise, the change looks good to me. > > In coresight_disable_helpers(), the return value of catu_disable() > isn't checked. > The -EBUSY return was used for consistency with other refcounted > disable functions > like tmc_disable_etf_sink() and tmc_disable_etr_sink(). I'm happy to > change it back > to 0 if you believe that would be the more accurate return value here. Please stick to 0 here. This indicates there was no errors w.r.t the current session. This is similar to what we do for TMC ETR for e.g. Suzuki > >> >> Thanks, >> Leo

4 months, 3 weeks

[PATCH] coresight: Break to error handler if connection can't be made

by James Clark

Change the return to a break so that the of_node_put()s in the error handler are hit on failure. Fixes: 3d4ff657e454 ("coresight: Dynamically add connections") Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Closes: https://lore.kernel.org/linux-arm-kernel/3b026b3f-2cb2-49ef-aa20-8b14220f53… Signed-off-by: James Clark <james.clark(a)linaro.org> --- drivers/hwtracing/coresight/coresight-platform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 8192ba3279f0..39851a13ff89 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -267,7 +267,8 @@ static int of_coresight_parse_endpoint(struct device *dev, new_conn = coresight_add_out_conn(dev, pdata, &conn); if (IS_ERR_VALUE(new_conn)) { fwnode_handle_put(conn.dest_fwnode); - return PTR_ERR(new_conn); + ret = PTR_ERR(new_conn); + break; } /* Connection record updated */ } while (0); --- base-commit: 5442d22da7dbff3ba8c6720fc6f23ea4934d402d change-id: 20250402-james-cs-ret-break-fix-ad38103af5f6 Best regards, -- James Clark <james.clark(a)linaro.org>

4 months, 3 weeks

Re: [PATCH v2 3/3] coresight: core: Disable helpers for devices that fail to enable

by Leo Yan

On Tue, Apr 01, 2025 at 06:22:46PM -0700, Yabin Cui wrote: > On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc(a)google.com> wrote: > > > > When enabling a SINK or LINK type coresight device fails, the > > associated helpers should be disabled. > > > > Signed-off-by: Yabin Cui <yabinc(a)google.com> > > Suggested-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> > > --- > > drivers/hwtracing/coresight/coresight-core.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > > index fb43ef6a3b1f..e3270d9b46c9 100644 > > --- a/drivers/hwtracing/coresight/coresight-core.c > > +++ b/drivers/hwtracing/coresight/coresight-core.c > > @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > /* Enable all helpers adjacent to the path first */ > > ret = coresight_enable_helpers(csdev, mode, path); > > if (ret) > > - goto err; > > + goto err_helper; > > /* > > * ETF devices are tricky... They can be a link or a sink, > > * depending on how they are configured. If an ETF has been > > @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > switch (type) { > > case CORESIGHT_DEV_TYPE_SINK: > > ret = coresight_enable_sink(csdev, mode, sink_data); > > - /* > > - * Sink is the first component turned on. If we > > - * failed to enable the sink, there are no components > > - * that need disabling. Disabling the path here > > - * would mean we could disrupt an existing session. > > - */ > > if (ret) > > - goto out; > > + goto err; > > break; > > case CORESIGHT_DEV_TYPE_SOURCE: > > /* sources are enabled from either sysFS or Perf */ > > @@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > out: > > return ret; > > err: > > + coresight_disable_helpers(csdev); Given it is unconditionally to enable helpers, I would suggest we disable helper unconditionally. Thus, we don't need to add a new 'err_helper' tag, simply reuse the 'err' tag would be fine. I would like to know if mainatiners have different opinions. Thanks, Leo > > +err_helper: > > coresight_disable_path_from(path, nd); > > goto out; > > } > > -- > > 2.49.0.472.ge94155a9ec-goog > >

4 months, 3 weeks

Re: [PATCH v2 2/3] coresight: catu: Prevent concurrent PERF and SYSFS mode enablement

by Leo Yan

On Tue, Apr 01, 2025 at 06:18:31PM -0700, Yabin Cui wrote: > A CATU device can be enabled for either PERF mode or SYSFS mode, but > not both simultaneously. Consider the following race condition: > > CPU 1 enables CATU for PERF mode. > CPU 2 enables CATU for SYSFS mode. It only increases refcnt. > CPU 2 enables ETR for SYSFS mode. > CPU 1 fails to enable ETR for PERF mode. > > This results in a CATU being enabled in PERF mode and an ETR enabled > in SYSFS mode, leading to unknown behavior. > > To prevent this situation, this patch checks enabled mode of a > CATU device before increasing its refcnt. Yes. We have also observed the same issue. I am currently working on refactoring the Arm CoreSight locking, my plan is to use coresight_path to maintain mode. Given it is uncommon case to use two modes concurrently in Arm CoreSight, I assume this is not an urgent issue. Could we defer this patch? My understanding is that this issue will be prevented with checking the path mode, rather than checking modes in seperate modules. To be clear, I am fine with other two patches in the series. Thanks, Leo > Signed-off-by: Yabin Cui <yabinc(a)google.com> > Suggested-by: James Clark <james.clark(a)linaro.org> > Suggested-by: Leo Yan <leo.yan(a)arm.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 6 +++++- > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index b1d490dd7249..0caf3a3e75ce 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, > CS_UNLOCK(catu_drvdata->base); > rc = catu_enable_hw(catu_drvdata, mode, data); > CS_LOCK(catu_drvdata->base); > - } > + if (!rc) > + catu_drvdata->mode = mode; > + } else if (catu_drvdata->mode != mode) > + rc = -EBUSY; > if (!rc) > csdev->refcnt++; > return rc; > @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused) > CS_UNLOCK(catu_drvdata->base); > rc = catu_disable_hw(catu_drvdata); > CS_LOCK(catu_drvdata->base); > + catu_drvdata->mode = CS_MODE_DISABLED; > } else { > rc = -EBUSY; > } > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h > index 755776cd19c5..ea406464f0a8 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.h > +++ b/drivers/hwtracing/coresight/coresight-catu.h > @@ -66,6 +66,7 @@ struct catu_drvdata { > struct coresight_device *csdev; > int irq; > raw_spinlock_t spinlock; > + enum cs_mode mode; > }; > > #define CATU_REG32(name, offset) \ > -- > 2.49.0.472.ge94155a9ec-goog >

4 months, 3 weeks

Re: [PATCH v2 1/3] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by Leo Yan

Hi Yabin, On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote: [...] > > @@ -486,12 +491,17 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) > > > > static int catu_disable(struct coresight_device *csdev, void *__unused) > > { > > - int rc; > > + int rc = 0; > > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > > > > - CS_UNLOCK(catu_drvdata->base); > > - rc = catu_disable_hw(catu_drvdata); > > - CS_LOCK(catu_drvdata->base); > > + if (--csdev->refcnt == 0) { > > + CS_UNLOCK(catu_drvdata->base); > > + rc = catu_disable_hw(catu_drvdata); > > + CS_LOCK(catu_drvdata->base); > > + } else { > > + rc = -EBUSY; This is not an error if the decremented reference counter is not zero. It should return 0. Otherwise, the change looks good to me. Thanks, Leo

4 months, 3 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight