Re: [PATCH v3 8/8] coresight-tpdm: Add msr register support for CMB
From: Tao Zhang
Date: Mon Jan 15 2024 - 09:16:25 EST
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@xxxxxxx>
Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
Signed-off-by: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
---
.../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@xxxxxxxxxxx>,
Tao Zhang (QUIC) <quic_taozha@xxxxxxxxxxx>
+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