[PATCH v3 8/8] coresight-tpdm: Add msr register support for CMB
Tao Zhang
quic_taozha at quicinc.com
Mon Jan 15 06:15:42 PST 2024
On 1/15/2024 5:20 PM, Suzuki K Poulose wrote:
> On 15/01/2024 06:20, Tao Zhang wrote:
>>
>> On 1/12/2024 5:43 PM, Suzuki K Poulose wrote:
>>> On 12/01/2024 09:12, Tao Zhang wrote:
>>>>
>>>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>>>
>>>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>>>
>>>>>>>>> Reviewed-by: James Clark <james.clark at arm.com>
>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha at quicinc.com>
>>>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao at quicinc.com>
>>>>>>>>> ---
>>>>>>>>> .../testing/sysfs-bus-coresight-devices-tpdm | 8 ++
>>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 86
>>>>>>>>> +++++++++++++++++++
>>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 16 +++-
>>>>>>>>> 3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>>> Accepts only one of the 2 values - 0 or 1.
>>>>>>>>> 0 : Disable the timestamp of all trace packets.
>>>>>>>>> 1 : Enable the timestamp of all trace packets.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>>>> +Date: September 2023
>>>>>>>>> +KernelVersion 6.7
>>>>>>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao at quicinc.com>,
>>>>>>>>> Tao Zhang (QUIC) <quic_taozha at quicinc.com>
>>>>>>>>> +Description:
>>>>>>>>> + (RW) Set/Get the MSR(mux select register) for the CMB
>>>>>>>>> subunit
>>>>>>>>> + TPDM.
>>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>>> @@ -86,6 +86,11 @@ static ssize_t
>>>>>>>>> tpdm_simple_dataset_show(struct device *dev,
>>>>>>>>> return -EINVAL;
>>>>>>>>> return sysfs_emit(buf, "0x%x\n",
>>>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>>>> + case CMB_MSR:
>>>>>>>>> + if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>>>> + return -EINVAL;
>>>>>>>>> + return sysfs_emit(buf, "0x%x\n",
>>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>>> }
>>>>>>>>> return -EINVAL;
>>>>>>>>> }
>>>>>>>>> @@ -162,6 +167,12 @@ static ssize_t
>>>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>>> else
>>>>>>>>> ret = -EINVAL;
>>>>>>>>> break;
>>>>>>>>> + case CMB_MSR:
>>>>>>>>> + if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>>>> + else
>>>>>>>>> + ret = -EINVAL;
>>>>>>>>
>>>>>>>>
>>>>>>>> minor nit: Could we not break from here instead of adding
>>>>>>>> return -EINVAL
>>>>>>>> for each case ? (I understand it has been done for the existing
>>>>>>>> cases.
>>>>>>>> But I think we should clean up all of that, including the ones
>>>>>>>> you added
>>>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>>>
>>>>>>> Sure, do I also need to change the DSB corresponding code? If
>>>>>>> so, how about
>>>>>>>
>>>>>>> if I add a new patch to the next patch series to change the
>>>>>>> previous existing cases?
>>>>>>
>>>>>> You could fix the existing cases as a preparatory patch of the
>>>>>> next version of this series. I can pick it up and push it to next
>>>>>> as I see fit.
>>>>>
>>>>> Got it. I will update this to the next patch series.
>>>>
>>>> Hi Suzuki,
>>>>
>>>>
>>>> Since the dataset data is configured with spin lock protection, it
>>>> needs to be unlock before return.
>>>>
>>>> List my modification below. Would you mind help review to see if it
>>>> is good for you.
>>>>
>>>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>>> struct device_attribute *attr,
>>>> const char *buf,
>>>> size_t size)
>>>> {
>>>> unsigned long val;
>>>>
>>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> struct tpdm_dataset_attribute *tpdm_attr =
>>>> container_of(attr, struct tpdm_dataset_attribute, attr);
>>>>
>>>> if (kstrtoul(buf, 0, &val))
>>>> return -EINVAL;
>>>>
>>>> spin_lock(&drvdata->spinlock);
>>>
>>> Use guard() to avoid explicit unlock on return and then you don't
>>> need the spin_unlock() everywhere. It would be done on return from the
>>> function implicitly.
>>>
>>>
>>>> switch (tpdm_attr->mem) {
>>>> case DSB_TRIG_PATT:
>>>
>>> With guard() in place:
>>>
>>> ret = -EINVAL;
>>>
>>> switch () {
>>> case XXX:
>>>
>>> if (tpdm_attr->idx < TPDM_XXXX_IDX) {
>>> drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>>> ret = size;
>>> }
>>> break;
>>> case ...
>>> ...
>>> }
>>>
>>> return ret;
>>
>> Thanks for your suggestion. I will update the code like below.
>>
>> I will update it in the next version of the patch series if it meets
>> your expectation.
>>
>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t size)
>> {
>> unsigned long val;
>> ssize_t ret = -EINVAL;
>>
>> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> struct tpdm_dataset_attribute *tpdm_attr =
>> container_of(attr, struct tpdm_dataset_attribute, attr);
>>
>> if (kstrtoul(buf, 0, &val))
>> return ret;
>>
>> guard(spinlock)(&drvdata->spinlock);
>> switch (tpdm_attr->mem) {
>> case DSB_TRIG_PATT:
>> if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) {
>> drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>> ret =size;
>> }
>> break;
>> case ...
>>
>> ...
>> }
>> return ret;
>> }
>>
>
> Yes that looks good to me. Please rebase this on to for-next/queue
> branch on the coresight repository.
Got it. Thanks for your mention.
Best,
Tao
>
> Suzuki
>
More information about the linux-arm-kernel
mailing list