[PATCH v3 RESEND 08/10] coresight: tmc: add a switch buffer function for byte-cntr
Jie Gan
jie.gan at oss.qualcomm.com
Tue Jul 22 20:29:16 PDT 2025
On 7/22/2025 10:09 PM, Mike Leach wrote:
> 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 at 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 at 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?
Should be ok with called if the the is DISABLED.
I will check the condition.
>
>> + __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.
It should be sysfs_buf->hwaddr here.
I made a mistake.
will fix it.
>
>> + 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?
Yes, there is a problem here. It's incorrectly put a strict condition here.
I will check the logic here and fix in next version.
Thanks,
Jie
>
>> + }
>> +
>> + 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
More information about the linux-arm-kernel
mailing list