On 08/04/2025 8:59 pm, Yabin Cui 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 | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fb43ef6a3b1f..a56ba9087538 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> * that need disabling. Disabling the path here
> * would mean we could disrupt an existing session.
> */
> - if (ret)
> + if (ret) {
> + coresight_disable_helpers(csdev);
Hi Yabin,
Unfortunately coresight_disable_helpers() takes a path pointer now so
this needs to be updated.
I tested with that change made and it works ok.
> goto out;
> + }
> break;
> case CORESIGHT_DEV_TYPE_SOURCE:
> /* sources are enabled from either sysFS or Perf */
> @@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> parent = list_prev_entry(nd, link)->csdev;
> child = list_next_entry(nd, link)->csdev;
> ret = coresight_enable_link(csdev, parent, child, source);
> - if (ret)
> + if (ret) {
> + coresight_disable_helpers(csdev);
> goto err;
> + }
> break;
> default:
> + coresight_disable_helpers(csdev);
Minor nit, you could collapse these last two into "goto
err_disable_helpers" and add another label before err: that disables
helpers before falling through to err:.
Other than that:
Reviewed-by: James Clark <james.clark(a)linaro.org>
> goto err;
> }
> }
On 08/04/2025 8:59 pm, Yabin Cui wrote:
> When tracing ETM data on multiple CPUs concurrently via the
> perf interface, the CATU device is shared across different CPU
> paths. This can lead to race conditions when multiple CPUs attempt
> to enable or disable the CATU device simultaneously.
>
> To address these race conditions, this patch introduces the
> following changes:
>
> 1. The enable and disable operations for the CATU device are not
> reentrant. Therefore, a spinlock is added to ensure that only
> one CPU can enable or disable a given CATU device at any point
> in time.
>
> 2. A reference counter is used to manage the enable/disable state
> of the CATU device. The device is enabled when the first CPU
> requires it and is only disabled when the last CPU finishes
> using it. This ensures the device remains active as long as at
> least one CPU needs it.
>
> Signed-off-by: Yabin Cui <yabinc(a)google.com>
> ---
> drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
> drivers/hwtracing/coresight/coresight-catu.h | 1 +
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index fa170c966bc3..30b78b2f8adb 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> void *data)
> {
> - int rc;
> + int rc = 0;
> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
>
Very minor nit only because you need to resend anyway, but there should
be a newline between the variable definitions and the code. Not sure why
checkpatch doesn't warn here.
> - CS_UNLOCK(catu_drvdata->base);
> - rc = catu_enable_hw(catu_drvdata, mode, data);
> - CS_LOCK(catu_drvdata->base);
> + if (csdev->refcnt == 0) {
> + CS_UNLOCK(catu_drvdata->base);
> + rc = catu_enable_hw(catu_drvdata, mode, data);
> + CS_LOCK(catu_drvdata->base);
> + }
> + if (!rc)
> + csdev->refcnt++;
> return rc;
> }
>
> @@ -486,12 +491,15 @@ 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) {
Hopefully this never underflows if disable is called again after a
failed enable. We could add a WARN_ON() but I think this is a general
case and not specific to these patches so is probably better to do later
as separate change.
Reviewed-by: James Clark <james.clark(a)linaro.org>
The Trace Network On Chip (TNOC) is an integration hierarchy which is a
hardware component that integrates the functionalities of TPDA and
funnels. It collects trace form subsystems and transfers to coresight
sink.
Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
---
Changes in v4:
- Fix dt_binding warning.
- update mask of trace_noc amba_id.
- Modify driver comments.
- rename TRACE_NOC_SYN_VAL to TRACE_NOC_SYNC_INTERVAL.
- Link to v3: https://lore.kernel.org/r/20250411-trace-noc-v3-0-1f19ddf7699b@quicinc.com
Changes in v3:
- Remove unnecessary sysfs nodes.
- update commit messages.
- Use 'writel' instead of 'write_relaxed' when writing to the register for the last time.
- Add trace_id ops.
- Link to v2: https://lore.kernel.org/r/20250226-trace-noc-driver-v2-0-8afc6584afc5@quici…
Changes in v2:
- Modified the format of DT binging file.
- Fix compile warnings.
- Link to v1: https://lore.kernel.org/r/46643089-b88d-49dc-be05-7bf0bb21f847@quicinc.com
---
Yuanfang Zhang (2):
dt-bindings: arm: Add device Trace Network On Chip definition
coresight: add coresight Trace Network On Chip driver
.../bindings/arm/qcom,coresight-tnoc.yaml | 111 ++++++++++++
drivers/hwtracing/coresight/Kconfig | 13 ++
drivers/hwtracing/coresight/Makefile | 1 +
drivers/hwtracing/coresight/coresight-tnoc.c | 191 +++++++++++++++++++++
drivers/hwtracing/coresight/coresight-tnoc.h | 34 ++++
5 files changed, 350 insertions(+)
---
base-commit: a2cc6ff5ec8f91bc463fd3b0c26b61166a07eb11
change-id: 20250403-trace-noc-f8286b30408e
Best regards,
--
Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
On 14/04/2025 11:06, Andy Shevchenko wrote:
> On Mon, Mar 31, 2025 at 02:40:51PM +0100, James Clark wrote:
>> On 31/03/2025 8:14 am, Andy Shevchenko wrote:
>
>> Reviewed-by: James Clark <james.clark(a)linaro.org>
>
> Thank you! Can it be applied now?
>
I will queue this via coresight tree, for v6.16
Cheers
Suzuki
The Trace Network On Chip (TNOC) is an integration hierarchy which is a
hardware component that integrates the functionalities of TPDA and
funnels. It collects trace form subsystems and transfers to coresight
sink.
Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
---
Changes in v3:
- Remove unnecessary sysfs nodes.
- update commit messages.
- Use 'writel' instead of 'write_relaxed' when writing to the register for the last time.
- Add trace_id ops.
- Link to v2: https://lore.kernel.org/r/20250226-trace-noc-driver-v2-0-8afc6584afc5@quici…
Changes in v2:
- Modified the format of DT binging file.
- Fix compile warnings.
- Link to v1: https://lore.kernel.org/r/46643089-b88d-49dc-be05-7bf0bb21f847@quicinc.com
---
Yuanfang Zhang (2):
dt-bindings: arm: Add device Trace Network On Chip definition
coresight: add coresight Trace Network On Chip driver
.../bindings/arm/qcom,coresight-tnoc.yaml | 111 ++++++++++++
drivers/hwtracing/coresight/Kconfig | 13 ++
drivers/hwtracing/coresight/Makefile | 1 +
drivers/hwtracing/coresight/coresight-tnoc.c | 186 +++++++++++++++++++++
drivers/hwtracing/coresight/coresight-tnoc.h | 34 ++++
5 files changed, 345 insertions(+)
---
base-commit: a2cc6ff5ec8f91bc463fd3b0c26b61166a07eb11
change-id: 20250403-trace-noc-f8286b30408e
Best regards,
--
Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
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
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
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;
> }
>