Re: [PATCH v4 11/11] coresight-tpdm: Add nodes for dsb msr support

[Tao Zhang <quic_taozha@quicinc.com>]

On 6/5/2023 6:24 PM, Suzuki K Poulose wrote:
> On 27/04/2023 10:00, Tao Zhang wrote:
>> Add the nodes for DSB subunit MSR(mux select register) support.
>> The TPDM MSR (mux select register) interface is an optional
>> interface and associated bank of registers per TPDM subunit.
>> The intent of mux select registers is to control muxing structures
>> driving the TPDM’s’ various subunit interfaces.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   | 15 ++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 53 
>> ++++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  3 ++
>>   3 files changed, 71 insertions(+)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 639b6fb8..f746f25 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -170,3 +170,18 @@ Description:
>>           Accepts only one of the 2 values -  0 or 1.
>>           0 : Set the DSB pattern type to value.
>>           1 : Set the DSB pattern type to toggle.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr
>> +Date:        March 2023
>> +KernelVersion    6.3
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +        (Write) Set the MSR(mux select register) of DSB tpdm. Read
>> +        the MSR(mux select register) of DSB tpdm.
>> +
>> +        Expected format is the following:
>> +        <integer1> <integer2>
>> +
>> +        Where:
>> +        <integer1> : Index number of MSR register
>> +        <integer2> : The value need to be written
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 627de36..5fe0bd5c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -240,6 +240,14 @@ static int tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>               if (!drvdata->dsb)
>>                   return -ENOMEM;
>>           }
>> +        if (!of_property_read_u32(drvdata->dev->of_node,
>> +               "qcom,dsb_msr_num", &drvdata->dsb->msr_num)) {
>> +            drvdata->dsb->msr = devm_kzalloc(drvdata->dev,
>> +                   (drvdata->dsb->msr_num * 
>> sizeof(*drvdata->dsb->msr)),
>> +                   GFP_KERNEL);
>> +            if (!drvdata->dsb->msr)
>> +                return -ENOMEM;
>> +        }
>>       }
>>         return 0;
>> @@ -765,6 +773,50 @@ static ssize_t dsb_trig_ts_store(struct device 
>> *dev,
>>   }
>>   static DEVICE_ATTR_RW(dsb_trig_ts);
>>   +static ssize_t dsb_msr_show(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned int i;
>> +    ssize_t size = 0;
>> +
>> +    if (drvdata->dsb->msr_num == 0)
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {
>
> Shouldn't this be  "i < drvdata->dsb->msr_num" ?
Yes, I will update it to the next patch series.
>
>
>> +        size += sysfs_emit_at(buf, size,
>> +                  "%u 0x%x\n", i, drvdata->dsb->msr[i]);
>> +    }
>> +    spin_unlock(&drvdata->spinlock);
>> +
>> +    return size;
>> +}
>> +
>> +static ssize_t dsb_msr_store(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  const char *buf,
>> +                  size_t size)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned int num, val;
>> +    int nval;
>> +
>> +    if (drvdata->dsb->msr_num == 0)
>> +        return -EINVAL;
>> +
>> +    nval = sscanf(buf, "%u %x", &num, &val);
>> +    if ((nval != 2) || (num >= (drvdata->dsb->msr_num - 1)))
>
> (num >= drvdata->dsb->msr_num) ?
Yes, I will update it to the next patch series.
>
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    drvdata->dsb->msr[num] = val;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(dsb_msr);
>> +
>>   static struct attribute *tpdm_dsb_attrs[] = {
>>       &dev_attr_dsb_mode.attr,
>>       &dev_attr_dsb_edge_ctrl.attr,
>> @@ -777,6 +829,7 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>       &dev_attr_dsb_trig_patt_mask.attr,
>>       &dev_attr_dsb_trig_ts.attr,
>>       &dev_attr_dsb_trig_type.attr,
>> +    &dev_attr_dsb_msr.attr,
>>       NULL,
>>   };
>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 9ad32a6..05e9f8e 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -18,6 +18,7 @@
>>   #define TPDM_DSB_XPMR(n)    (0x7E8 + (n * 4))
>>   #define TPDM_DSB_EDCR(n)    (0x808 + (n * 4))
>>   #define TPDM_DSB_EDCMR(n)    (0x848 + (n * 4))
>> +#define TPDM_DSB_MSR(n)        (0x980 + (n * 4))
>>     /* Enable bit for DSB subunit */
>>   #define TPDM_DSB_CR_ENA        BIT(0)
>> @@ -113,6 +114,8 @@ struct dsb_dataset {
>>       u32                trig_patt_mask[TPDM_DSB_MAX_PATT];
>>       bool            trig_ts;
>>       bool            trig_type;
>> +    u32                msr_num;
>> +    u32                *msr;
>>   };
>>     /**
>
>
> Where/when do we write to these registers in the DSB ?

DSB MSR registers should be written in the DSB TPDM enablement function.

I will update this to the next patch series.


Best

Tao

>
> Suzuki