Message-ID: <cef984d5-f369-4892-b970-a71285c2ebc5@quicinc.com>
Date: Wed, 12 Mar 2025 09:20:44 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: Mike Leach <mike.leach@...aro.org>
CC: Suzuki K Poulose <suzuki.poulose@....com>,
        James Clark
	<james.clark@...aro.org>,
        Alexander Shishkin
	<alexander.shishkin@...ux.intel.com>,
        Maxime Coquelin
	<mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Tingwei Zhang
	<quic_tingweiz@...cinc.com>,
        Jinlong Mao <quic_jinlmao@...cinc.com>, <coresight@...ts.linaro.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP
 offset of ETR buffer



On 3/12/2025 12:49 AM, Mike Leach wrote:
> Hi,
> 
> On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@...cinc.com> 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@...cinc.com>
>> ---
>>   .../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 
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 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.


> 
>> +       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
>>
> 
> 

Powered by blists - more mailing lists