linux-kernel - Re: [PATCH v1 2/3] coresight: tpda: add function to configure TPDA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c78a157f-d8d8-4647-8a2b-4409489633db@oss.qualcomm.com>
Date: Tue, 26 Aug 2025 17:33:43 +0800
From: Jie Gan <jie.gan@....qualcomm.com>
To: James Clark <james.clark@...aro.org>, Jie Gan <jie.gan@....qualcomm.com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach
 <mike.leach@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Tingwei Zhang <tingwei.zhang@....qualcomm.com>
Subject: Re: [PATCH v1 2/3] coresight: tpda: add function to configure
 TPDA_SYNCR register



On 8/26/2025 5:20 PM, James Clark wrote:
> 
> 
> On 26/08/2025 8:01 am, Jie Gan wrote:
>> From: Tao Zhang <tao.zhang@....qualcomm.com>
>>
>> The TPDA_SYNCR register defines the frequency at which TPDA generates
>> ASYNC packets, enabling userspace tools to accurately parse each valid
>> packet.
>>
>> Signed-off-by: Tao Zhang <tao.zhang@....qualcomm.com>
>> Co-developed-by: Jie Gan <jie.gan@....qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@....qualcomm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpda.c | 15 +++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h |  1 +
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>> hwtracing/coresight/coresight-tpda.c
>> index cc254d53b8ec..9e623732d1e7 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -189,6 +189,18 @@ static void tpda_enable_pre_port(struct 
>> tpda_drvdata *drvdata)
>>           writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>>   }
>> +static void tpda_enable_post_port(struct tpda_drvdata *drvdata)
>> +{
>> +    uint32_t val;
> 
> Minor nit: this is inconsistent with u32 used elsewhere in this file.

Will fix it in next version.

> 
>> +
>> +    val = readl_relaxed(drvdata->base + TPDA_SYNCR);
>> +    /* Clear the mode */
>> +    val = val & ~TPDA_MODE_CTRL;
> 
> &=

Will fix.

> 
>> +    /* Program the counter value */
>> +    val = val | 0xFFF;
> 
> |=

Will fix.

> 
> Defining a field would be a bit nicer here. Like:
> 
> val |= FIELD_PREP(TPDA_SYNCR_COUNTER, UINT32_MAX);

That's better, forgot to use the proper Macro. I will re-check all codes 
again to update all possible fixes.

> 
> Assuming you wanted to set all bits, and 0xFFF isn't some specific value.

Yes, this field has 12 bits and we prefer the max value to prevent to 
generate too many ASYNC packets. This field indicates a count value for 
number of bytes. Once the the count reaches the number, a ASYNC packet 
will be generated.

> 
>> +    writel_relaxed(val, drvdata->base + TPDA_SYNCR);
>> +}
>> +
>>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>>   {
>>       u32 val;
>> @@ -227,6 +239,9 @@ static int __tpda_enable(struct tpda_drvdata 
>> *drvdata, int port)
>>           tpda_enable_pre_port(drvdata);
>>       ret = tpda_enable_port(drvdata, port);
>> +    if (!drvdata->csdev->refcnt)
>> +        tpda_enable_post_port(drvdata);
> 
> Any reason this can't be done on tpda_enable_pre_port()? It has the same 
> logic where it's only done once for the first port.
> 
> If it can't be done there you should add a comment saying why it must be 
> done after enabling the first port.

This register only affect the port which already be enabled. That's why 
we add it to enable_post_port. But as you mentioned, we can put the 
logic into enable_pre_port without side effect.

I think it's ok to move the logic to enable_pre_port to simply codes here.

Thanks,
Jie

> 
>> +
>>       CS_LOCK(drvdata->base);
>>       return ret;
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>> hwtracing/coresight/coresight-tpda.h
>> index b651372d4c88..00d146960d81 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -9,6 +9,7 @@
>>   #define TPDA_CR            (0x000)
>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>   #define TPDA_FPID_CR        (0x084)
>> +#define TPDA_SYNCR        (0x08C)
>>   /* Cross trigger FREQ packets timestamp bit */
>>   #define TPDA_CR_FREQTS        BIT(2)
> 
>