On 15/01/2024 09:55, James Clark 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@arm.com > Signed-off-by: Tao Zhang quic_taozha@quicinc.com > Signed-off-by: Mao Jinlong quic_jinlmao@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@quicinc.com, Tao > Zhang (QUIC) quic_taozha@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); switch (tpdm_attr->mem) { case DSB_TRIG_PATT: if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) drvdata->dsb->trig_patt[tpdm_attr->idx] = val; else { spin_unlock(&drvdata->spinlock); return -EINVAL; } case DSB_TRIG_PATT_MASK: if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) drvdata->dsb->trig_patt_mask[tpdm_attr->idx] = val; else{ spin_unlock(&drvdata->spinlock); return -EINVAL; } case DSB_PATT: if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) drvdata->dsb->patt_val[tpdm_attr->idx] = val; else{ spin_unlock(&drvdata->spinlock); return -EINVAL; } case DSB_PATT_MASK: if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) drvdata->dsb->patt_mask[tpdm_attr->idx] = val; else{ spin_unlock(&drvdata->spinlock); return -EINVAL; } case DSB_MSR: if (tpdm_attr->idx < drvdata->dsb_msr_num) drvdata->dsb->msr[tpdm_attr->idx] = val; else{ spin_unlock(&drvdata->spinlock); return -EINVAL; } default: spin_unlock(&drvdata->spinlock); return -EINVAL; } return size;
Best,
Tao
This looks like a good fit for the new guard(spinlock)(&drvdata->spinlock) thing. Then there is no need to do all the manual unlocking.
Oh I see Suzuki already suggested it, nevermind.