Re: [PATCH v3 RESEND 08/10] coresight: tmc: add a switch buffer function for byte-cntr

Hi,

This buffer swap code looks OK in principle. The ETR is stopped, memory synced and set to be read. See other comments inline.

On Mon, 14 Jul 2025 at 07:31, Jie Gan jie.gan@oss.qualcomm.com wrote:

Switching the sysfs_buf when current buffer is full or the timeout is triggered and resets rrp and rwp registers after switched the buffer. Disable the ETR device if it cannot find an available buffer to switch.

Signed-off-by: Jie Gan jie.gan@oss.qualcomm.com

.../hwtracing/coresight/coresight-tmc-etr.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 2b73bd8074bb..3e3e1b5e78ca 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1287,6 +1287,58 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) return ret ? ERR_PTR(ret) : drvdata->sysfs_buf; }

+static bool tmc_byte_cntr_switch_buffer(struct tmc_drvdata *drvdata,

•                                   struct ctcu_byte_cntr *byte_cntr_data)
  

+{

This entire function should be in one of the byte_cntr source files, not in the main etr files. Please keep byte cntr code separate as far as possible

•   struct etr_buf_node *nd, *next, *curr_node, *picked_node;
  

•   struct etr_buf *curr_buf = drvdata->sysfs_buf;
  

•   bool found_free_buf = false;
  

•   if (WARN_ON(!drvdata || !byte_cntr_data))
  

•           return found_free_buf;
  

•   /* Stop the ETR before we start the switching process */
  

•   if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
  

Can this function be called if the mode is not CS_MODE_SYSFS?

•           __tmc_etr_disable_hw(drvdata);
  

•   list_for_each_entry_safe(nd, next, &drvdata->etr_buf_list, node) {
  

•           /* curr_buf is free for next round */
  

•           if (nd->sysfs_buf == curr_buf) {
  

•                   nd->is_free = true;
  

•                   curr_node = nd;
  

•           }
  

•           if (!found_free_buf && nd->is_free && nd->sysfs_buf != curr_buf) {
  

•                   if (nd->reading)
  

•                           continue;
  

•                   picked_node = nd;
  

•                   found_free_buf = true;
  

•           }
  

•   }
  

•   if (found_free_buf) {
  

•           curr_node->reading = true;
  

•           curr_node->pos = 0;
  

•           drvdata->reading_node = curr_node;
  

•           drvdata->sysfs_buf = picked_node->sysfs_buf;
  

•           drvdata->etr_buf = picked_node->sysfs_buf;
  

•           picked_node->is_free = false;
  

•           /* Reset irq_cnt for next etr_buf */
  

•           atomic_set(&byte_cntr_data->irq_cnt, 0);
  

•           /* Reset rrp and rwp when the system has switched the buffer*/
  

•           CS_UNLOCK(drvdata->base);
  

•           tmc_write_rrp(drvdata, 0);
  

•           tmc_write_rwp(drvdata, 0);
  

This cannot possibly be correct. RWP / RRP are pointers into the system memory where the ETR stores data.

•           CS_LOCK(drvdata->base);
  

•           /* Restart the ETR when we find a free buffer */
  

•           if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
  

•                   __tmc_etr_enable_hw(drvdata);
  

What happens if the ETR is not restarted? Using __tmc_etr_disable_hw() is correct for this use case, but if you do not restart then the extra shutdown that would ordinarily happen in tmc_etr_disable_hw() does not occur. How is this handled in the rest of the update?

•   }
  

•   return found_free_buf;
  

+}

static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) { int ret = 0; -- 2.34.1

Regards

Mike

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