Received-SPF: pass (google.com: domain of linux-kernel+bounces-32320-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <5a2bf1ba-d22b-4227-87d2-7e94f2a60866@quicinc.com>
Date: Mon, 22 Jan 2024 12:27:58 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 06/10] coresight-tpdm: Add support to configure CMB
Content-Language: en-US
To: Suzuki K Poulose <suzuki.poulose@arm.com>,
        Mathieu Poirier
	<mathieu.poirier@linaro.org>,
        Alexander Shishkin
	<alexander.shishkin@linux.intel.com>,
        Konrad Dybcio <konradybcio@gmail.com>,
        Mike Leach <mike.leach@linaro.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Jinlong Mao <quic_jinlmao@quicinc.com>,
        Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>,
        <coresight@lists.linaro.org>, <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        Tingwei Zhang <quic_tingweiz@quicinc.com>,
        Yuanfang Zhang <quic_yuanfang@quicinc.com>,
        Trilok Soni
	<quic_tsoni@quicinc.com>,
        Song Chai <quic_songchai@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <andersson@kernel.org>
References: <1705634583-17631-1-git-send-email-quic_taozha@quicinc.com>
 <1705634583-17631-7-git-send-email-quic_taozha@quicinc.com>
 <03e056cd-bdbc-4807-b86c-0f5b554aac73@arm.com>
From: Tao Zhang <quic_taozha@quicinc.com>
In-Reply-To: <03e056cd-bdbc-4807-b86c-0f5b554aac73@arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 1/19/2024 7:53 PM, Suzuki K Poulose wrote:
> On 19/01/2024 03:22, Tao Zhang wrote:
>> TPDM CMB subunits support two forms of CMB data set element creation:
>> continuous and trace-on-change collection mode. Continuous change
>> creates CMB data set elements on every CMBCLK edge. Trace-on-change
>> creates CMB data set elements only when a new data set element differs
>> in value from the previous element in a CMB data set. Set CMB_CR.MODE
>> to 0 for continuous CMB collection mode. Set CMB_CR.MODE to 1 for
>> trace-on-change CMB collection mode.
>>
>> Reviewed-by: James Clark <james.clark@arm.com>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpdm  | 14 +++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 61 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h  | 12 ++++
>>   3 files changed, 87 insertions(+)
>>
>> diff --git 
>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 4dd49b159543..3ae21ccf3f29 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -170,3 +170,17 @@ Contact:    Jinlong Mao (QUIC) 
>> <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_t
>>   Description:
>>           (RW) Set/Get the MSR(mux select register) for the DSB subunit
>>           TPDM.
>> +
>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_mode
>> +Date:        March 2023
>> +KernelVersion    6.7
>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>> (QUIC) <quic_taozha@quicinc.com>
>> +Description:    (Write) Set the data collection mode of CMB tpdm. 
>> Continuous
>> +        change creates CMB data set elements on every CMBCLK edge.
>> +        Trace-on-change creates CMB data set elements only when a new
>> +        data set element differs in value from the previous element
>> +        in a CMB data set.
>> +
>> +        Accepts only one of the 2 values -  0 or 1.
>> +        0 : Continuous CMB collection mode.
>> +        1 : Trace-on-change CMB collection mode.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 424a2f724d82..b55aee65a856 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -137,6 +137,18 @@ static umode_t tpdm_dsb_is_visible(struct 
>> kobject *kobj,
>>       return 0;
>>   }
>>   +static umode_t tpdm_cmb_is_visible(struct kobject *kobj,
>> +                   struct attribute *attr, int n)
>> +{
>> +    struct device *dev = kobj_to_dev(kobj);
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    if (drvdata && tpdm_has_cmb_dataset(drvdata))
>> +        return attr->mode;
>> +
>> +    return 0;
>> +}
>> +
>>   static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,
>>                          struct attribute *attr, int n)
>>   {
>> @@ -161,6 +173,9 @@ static void tpdm_reset_datasets(struct 
>> tpdm_drvdata *drvdata)
>>           drvdata->dsb->trig_ts = true;
>>           drvdata->dsb->trig_type = false;
>>       }
>> +
>> +    if (tpdm_has_cmb_dataset(drvdata))
>
> This could simply be gated on drvdata->cmb for extra safety ?
>
>     if (drvdata->cmb)
Sure, can I also use "if (drvdata->dsb) " instead of "if 
(tpdm_has_dsb_dataset(drvdata))" and update in the same patch?
>
>> +        memset(drvdata->cmb, 0, sizeof(struct cmb_dataset));
>>   }
>>     static void set_dsb_mode(struct tpdm_drvdata *drvdata, u32 *val)
>> @@ -389,6 +404,12 @@ static int tpdm_datasets_setup(struct 
>> tpdm_drvdata *drvdata)
>>           if (!drvdata->dsb)
>>               return -ENOMEM;
>>       }
>> +    if (tpdm_has_cmb_dataset(drvdata) && (!drvdata->cmb)) {
>> +        drvdata->cmb = devm_kzalloc(drvdata->dev,
>> +                        sizeof(*drvdata->cmb), GFP_KERNEL);
>> +        if (!drvdata->cmb)
>> +            return -ENOMEM;
>> +    }
>>       tpdm_reset_datasets(drvdata);
>>         return 0;
>> @@ -727,6 +748,35 @@ static ssize_t dsb_trig_ts_store(struct device 
>> *dev,
>>   }
>>   static DEVICE_ATTR_RW(dsb_trig_ts);
>>   +static ssize_t cmb_mode_show(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%x\n",
>> +              drvdata->cmb->trace_mode);
>
> minor nit: Don't need to split the line here. Also, for completeness, 
> you need to read it under spinlock, use guard() to unlock implicitly.

Sure, I will not split the line here in the next patch series.

I didn't add spinlock here because the show function only read the value.

Do you mean I need to add the spinlock to all of the show function?

>
>> +
>> +}
>> +
>> +static ssize_t cmb_mode_store(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  const char *buf,
>> +                  size_t size)
>> +{
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long trace_mode;
>> +
>> +    if ((kstrtoul(buf, 0, &trace_mode)) || (trace_mode & ~1UL))
>
> minor nit: drop () around kstrtoul()

Sure, I will update in the next patch series.


Best,

Tao

>
> Rest looks fine.
>
> Suzuki
>
>> +        return -EINVAL;
>> +
>> +    spin_lock(&drvdata->spinlock);
>> +    drvdata->cmb->trace_mode = trace_mode;
>> +    spin_unlock(&drvdata->spinlock);
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(cmb_mode);
>> +
>>   static struct attribute *tpdm_dsb_edge_attrs[] = {
>>       &dev_attr_ctrl_idx.attr,
>>       &dev_attr_ctrl_val.attr,
>> @@ -843,6 +893,11 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>       NULL,
>>   };
>>   +static struct attribute *tpdm_cmb_attrs[] = {
>> +    &dev_attr_cmb_mode.attr,
>> +    NULL,
>> +};
>> +
>>   static struct attribute_group tpdm_dsb_attr_grp = {
>>       .attrs = tpdm_dsb_attrs,
>>       .is_visible = tpdm_dsb_is_visible,
>> @@ -872,6 +927,11 @@ static struct attribute_group tpdm_dsb_msr_grp = {
>>       .name = "dsb_msr",
>>   };
>>   +static struct attribute_group tpdm_cmb_attr_grp = {
>> +    .attrs = tpdm_cmb_attrs,
>> +    .is_visible = tpdm_cmb_is_visible,
>> +};
>> +
>>   static const struct attribute_group *tpdm_attr_grps[] = {
>>       &tpdm_attr_grp,
>>       &tpdm_dsb_attr_grp,
>> @@ -879,6 +939,7 @@ static const struct attribute_group 
>> *tpdm_attr_grps[] = {
>>       &tpdm_dsb_trig_patt_grp,
>>       &tpdm_dsb_patt_grp,
>>       &tpdm_dsb_msr_grp,
>> +    &tpdm_cmb_attr_grp,
>>       NULL,
>>   };
>>   diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index a442d9c6e4ac..2af92c270ed1 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -14,6 +14,8 @@
>>     /* Enable bit for CMB subunit */
>>   #define TPDM_CMB_CR_ENA        BIT(0)
>> +/* Trace collection mode for CMB subunit */
>> +#define TPDM_CMB_CR_MODE    BIT(1)
>>     /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>> @@ -181,6 +183,14 @@ struct dsb_dataset {
>>       bool            trig_type;
>>   };
>>   +/**
>> + * struct cmb_dataset
>> + * @trace_mode:       Dataset collection mode
>> + */
>> +struct cmb_dataset {
>> +    u32            trace_mode;
>> +};
>> +
>>   /**
>>    * struct tpdm_drvdata - specifics associated to an TPDM component
>>    * @base:       memory mapped base address for this component.
>> @@ -190,6 +200,7 @@ struct dsb_dataset {
>>    * @enable:     enable status of the component.
>>    * @datasets:   The datasets types present of the TPDM.
>>    * @dsb         Specifics associated to TPDM DSB.
>> + * @cmb         Specifics associated to TPDM CMB.
>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>>    */
>>   @@ -201,6 +212,7 @@ struct tpdm_drvdata {
>>       bool            enable;
>>       unsigned long        datasets;
>>       struct dsb_dataset    *dsb;
>> +    struct cmb_dataset    *cmb;
>>       u32            dsb_msr_num;
>>   };
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org