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-tpdmindex 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.cindex 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 addedin Patch 5. Similarly for the dataset_show()Sure, do I also need to change the DSB corresponding code? If so, how aboutif 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 thefunction 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
