Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

From: Jie Gan
Date: Thu Mar 13 2025 - 05:02:59 EST

Next message: Andy Shevchenko: "Re: [PATCH] net-sysfs: fix error handling in netdev_register_kobject()"
Previous message: Mantas Pucka: "[PATCH] wifi: ath12k: fix 5GHz operation on wideband QCN9274 radios"
In reply to: Jie Gan: "Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer"
Next in thread: Jie Gan: "[PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 3/13/2025 9:32 AM, Jie Gan wrote:

On 3/12/2025 8:54 PM, Mike Leach wrote:

Hi Jie,

On Wed, 12 Mar 2025 at 01:21, Jie Gan <quic_jiegan@xxxxxxxxxxx> wrote:

On 3/12/2025 12:49 AM, Mike Leach wrote:

Hi,

On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@xxxxxxxxxxx> wrote:

The new functions calculate and return the offset to the write pointer of
the ETR buffer based on whether the memory mode is SG, flat or reserved.
The functions have the RWP offset can directly read data from ETR buffer,
enabling the transfer of data to any required location.

Signed-off-by: Jie Gan <quic_jiegan@xxxxxxxxxxx>
---
   .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++ ++++++
   drivers/hwtracing/coresight/coresight-tmc.h   | 1 +
   2 files changed, 41 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ drivers/hwtracing/coresight/coresight-tmc-etr.c
index eda7cdad0e2b..ec636ab1fd75 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
   }
   EXPORT_SYMBOL_GPL(tmc_free_sg_table);

+static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
+       u64 rwp;
+

It is not valid to read RWP if the TMC is running. It must be in the
stopped or disabled state - see the specifications for TMC /ETR

It is likely that CSUNLOCK / CSLOCK are needed here too, along with
the spinlock that protects drvdata

See the code in coresight_tmc_etr.c :-

e.g. in

tmc_update_etr_buffer()

...
<take spinlock>
...
CS_UNLOCK(drvdata->base);
tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
flushed to memory - essential to ensure full formatted frame is in
memory.
tmc_sync_etr_buf(drvdata); // this function reads rwp.
CS_LOCK(drvdata->base);
<release spinlokc>

This type of program flow is common to both sysfs and perf handling of
TMC buffers.

Hi Mike,

I am fully understood your point here.

The function is designed this way to read the w_offset (which may not be
entirely accurate because the etr buffer is not synced) when the

Why would you ever base memory access on a pointer that is not
entirely accurate?

The manuals for TMC/ETR all state that reads to both RWP and RRP when
the ETR is running return unknown values. These cannot be used to
access the buffer, or determine how much of the buffer has been used
on a running ETR.

The ETR specification specifically states that it is not permitted to
read the buffer data while the ETR is running, when configured in
circular buffer mode - which is the mode used in the TMC-ETR linux
drivers.

Reading the buffer while ETR is running is only permitted if
configured in Software FIFO mode 2 - were the ETR will stop on full
and stall incoming trace until some data is read out, signalled to the
ETR via the RURP.

Hi Mike,

I appreciate for your patient explanation.

I was wrong about read data from etr_buffer. I must follow the specification to design a method to reading buffer while ETR is running.

How about the following method:

1. The byte-cntr interrupt handler will count the IRQ triggered number when byte-cntr file node is opened.
2. Read the buffer after the ETR is stopped(full or stopped manually) according to the counted number. we got the etr->offset, etr->size and the counted number, so we can calculate the offset where starts to read.
3. Restart the ETR to keep counting the number of IRQ triggers.

Thanks,
Jie

Hi Mike,

Please ignore my previous proposal.

Let me clarify the purpose of the byte-cntr function to ensure there are no misunderstandings from my previous comments.

First, we definitely need to change the method for reading the rwp pointer from the RWP register.The new method ensures that we obtain a valid rwp value.

code snippet:
long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
{
struct etr_buf *etr_buf;
unsigned long flags;
long w_offset;

if (WARN_ON(!drvdata->sysfs_buf))
return -EINVAL;

spin_lock_irqsave(&drvdata->spinlock, flags);
etr_buf = drvdata->sysfs_buf;
if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
__tmc_etr_disable_hw(drvdata);

if (etr_buf->mode == ETR_MODE_ETR_SG)
w_offset = tmc_sg_get_rwp_offset(drvdata);
else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
w_offset = tmc_flat_resrv_get_rwp_offset(drvdata);
else
w_offset = -EINVAL;

if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
__tmc_etr_enable_hw(drvdata);

spin_unlock_irqrestore(&drvdata->spinlock, flags);

return w_offset;
}

The rwp offset is necessary because the length of the trace data transferred by the byte-cntr depends on the IRQ count number and the rwp value is read (as a benchmark to indicate where we start from) when the interrupt is enabled.

With a valid rwp value, we can copy trace data from the ETR buffer (The trace data in the ETR buffer can still be read using tmc_read) based on the IRQ count number.

We need the byte-cntr function to continue transferring data from the ETR buffer to userspace (because the etr buffer has some limitations to store trace data) while the ETR is running but we just utilize the rwp value as a benchmark to copy trace data from etr buffer, is this acceptable?

Please correct me if I am wrong.

Thanks,
Jie

I also note that you are reading back the etr_buf data without doing
any dma_sync operations that the perf and sysfs methods in the driver
do, after stopping the tmc.

byte-cntr devnode is opened, aiming to reduce the length of redundant
trace data. In this case, we cannot ensure the TMC is stopped or
disabled.

The specification requires that you must ensure the TMC is stopped to
read these registers.

The byte-cntr only requires an offset to know where it can
start before the expected trace data gets into ETR buffer.

The w_offset is also read when the byte-cntr function stops, which
occurs after the TMC is disabled.

Maybe this is not a good idea and I should read r_offset upon open?
The primary goal for byte-cntr is trying to transfer useful trace data
from the ETR buffer to the userspace, if we start from r_offset, a large
number of redundant trace data which the user does not expect will be
transferred simultaneously.

It is difficult to justify adding code to a driver that does not
correspond to the specification of the hardware device.

Regards

Mike

+       rwp = tmc_read_rwp(drvdata);
+       return rwp - paddr;
+}
+
+static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+       struct etr_buf *etr_buf = drvdata->sysfs_buf;
+       struct etr_sg_table *etr_table = etr_buf->private;
+       struct tmc_sg_table *table = etr_table->sg_table;
+       long w_offset;
+       u64 rwp;
+

Same comments as above

+       rwp = tmc_read_rwp(drvdata);
+       w_offset = tmc_sg_get_data_page_offset(table, rwp);
+
+       return w_offset;
+}
+
+/*
+ * Retrieve the offset to the write pointer of the ETR buffer based on whether
+ * the memory mode is SG, flat or reserved.
+ */
+long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+       struct etr_buf *etr_buf = drvdata->sysfs_buf;
+

As this is an exported function, please ensure that the inputs are
valid - check the pointers

Sure, will do.

Thanks,
Jie

Code to ensure TMC is flushed and stopped could be inserted here.

Regards

Mike

+       if (etr_buf->mode == ETR_MODE_ETR_SG)
+               return tmc_sg_get_rwp_offset(drvdata);
+       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
+               return tmc_flat_resrv_get_rwp_offset(drvdata);
+       else
+               return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
+
   /*
    * Alloc pages for the table. Since this will be used by the device,
    * allocate the pages closer to the device (i.e, dev_to_node(dev)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ hwtracing/coresight/coresight-tmc.h
index b48bc9a01cc0..baedb4dcfc3f 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
                                     enum cs_mode mode, void *data);
   extern const struct attribute_group coresight_etr_group;
+long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);

   #endif
--
2.34.1

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Next message: Andy Shevchenko: "Re: [PATCH] net-sysfs: fix error handling in netdev_register_kobject()"
Previous message: Mantas Pucka: "[PATCH] wifi: ath12k: fix 5GHz operation on wideband QCN9274 radios"
In reply to: Jie Gan: "Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer"
Next in thread: Jie Gan: "[PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]