Re: [PATCH v3 6/8] coresight-tpdm: Add timestamp control register support for the CMB

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 12/01/2024 02:42, Tao Zhang wrote:
> 
> On 1/8/2024 6:42 PM, Suzuki K Poulose wrote:
>> On 05/01/2024 07:49, Tao Zhang wrote:
>>>
>>> On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
>>>> On 25/12/2023 01:55, Tao Zhang wrote:
>>>>>
>>>>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>>>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>>>>
>>>>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:

...

>>>>>>>>>>>                     char *buf)
>>>>>>>>>>>   {
>>>>>>>>>>>       struct tpdm_drvdata *drvdata = 
>>>>>>>>>>> dev_get_drvdata(dev->parent);
>>>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>>>   -    return sysfs_emit(buf, "%u\n",
>>>>>>>>>>> -             (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> +                 (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> +
>>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> +        size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> +                 (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>>>>
>>>>>>>>>> Why does this need to show two values ? This must only show 
>>>>>>>>>> ONE value.
>>>>>>>>>> How you deduce that might be based on the availability of the 
>>>>>>>>>> feature
>>>>>>>>>> set. Or store the TS value in the drvdata and use that instead 
>>>>>>>>>> for
>>>>>>>>>> controlling CMB/DSB.
>>>>>>>>>
>>>>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can 
>>>>>>>>> I separate them
>>>>>>>>
>>>>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>>>>
>>>>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>>>>
>>>>>>>> I am not an expert on the usage scenario of the same. So, if 
>>>>>>>> you/Qcomm
>>>>>>>> thinks the users need separate, fine grained control for timestamp
>>>>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>>>>
>>>>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like 
>>>>>>>>> below.
>>>>>>>>>
>>>>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>>>>
>>>>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>>>>
>>>>>>>>>
>>>>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>>>>
>>>>>>>>> Is this design appropriate?
>>>>>>>>
>>>>>>>>
>>>>>>>> Otherwise, stick to single enable_ts : which enables the ts for 
>>>>>>>> both
>>>>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for 
>>>>>>>> both
>>>>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>>>>
>>>>>>> We have a very special case, such as the TPDM supporting both CMB 
>>>>>>> and
>>>>>>>
>>>>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>>>>
>>>>>>> Can I use the data bit to instruct whether timestamp is enabled 
>>>>>>> for CMB/DSB or not? For example,
>>>>>>>
>>>>>>> size = sysfs_emit(buf, "%u\n",
>>>>>>>                  (unsigned int)(drvdata->dsb->patt_ts << 1 | 
>>>>>>> drvdata->cmb->patt_ts));
>>>>>>>
>>>>>>> Thus, this value can instruct the following situations.
>>>>>>>
>>>>>>> 0 - TS is disabled for both CMB/DSB
>>>>>>>
>>>>>>> 1 - TS is enabled for CMB
>>>>>>>
>>>>>>> 2 - TS is enabled for DSB
>>>>>>>
>>>>>>> 3 - TS is enabled for both
>>>>>>>
>>>>>>> Is this approach acceptable?
>>>>>>>
>>>>>>
>>>>>> No, please stick to separate controls for TS. Do not complicate
>>>>>> the user interface.
>>>>>>
>>>>>> i.e.,
>>>>>> tpdm0/dsb_patt/enable_ts
>>>>>> tpdm0/cmb_patt/enable_ts
>>>>>
>>>>> We need to be able to control/show dsb and cmb timestamp enablement 
>>>>> independently.
>>>>>
>>>>> Can we achieve this requirement if we use a sysfs file with the 
>>>>> same name?
>>>>
>>>> They are independent and in their respective directory (group) for 
>>>> CMB and DSB. What am I missing ?
>>>> e.g., if you want to enable TS for DSB, you do :
>>>>
>>>> $ echo 1 > dsb_patt/enable_ts
>>>>
>>>> And that only works for DSB not for CMB.
>>>
>>> We have a special case that the TPDM supports both DSB and CMB 
>>> dataset. In this special case, when we
>>>
>>> issue this command to enable timestamp, it will call enable_ts_store 
>>> API, right?
>>>
>>>      if (tpdm_has_dsb_dataset(drvdata))
>>>          drvdata->dsb->patt_ts = !!val;
>>>
>>>      if (tpdm_has_cmb_dataset(drvdata))
>>>          drvdata->cmb->patt_ts = !!val;
>>
>> I don't understand. If they both are under different subgroups, why
>> should they be conflicting ? Are you not able to distinguish them, when
>>  you creat those attributes ? i.e., create two different "attributes" ?
> 
> Yes, although some TPDMs can support both CMB dataset and DSB dataset, 
> we need to configure them separately
> 
> in some scenarios. Based on your suggestion, I want to use the following 
> approach to implement it.
> 
> See below.
> 
>>
>> See below.
>>
>>> Since this special TPDM supports both DSB and CMB dataset, both DSB 
>>> patt_ts and CMB patt_ts will be set
>>>
>>> in this case even if I only configure the file in the DSB directory, 
>>> right?
>>>
>>> This is the problem we have now.
>>>
>>
>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +    return size;
>>>>>>>>>>>   }
>>>>>>>>>>>     /*
>>>>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct 
>>>>>>>>>>> device *dev,
>>>>>>>>>>>           return -EINVAL;
>>>>>>>>>>>         spin_lock(&drvdata->spinlock);
>>>>>>>>>>> -    drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> +    if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> +        drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> +        drvdata->cmb->patt_ts = !!val;
>>>>>>>>>>>       spin_unlock(&drvdata->spinlock);
>>>>>>>>>>> +
>>>>>>>>>>>       return size;
>>>>>>>>>>>   }
>>>>>>>>>>>   static DEVICE_ATTR_RW(enable_ts);
>>
>> Do not overload the same for both DSB and CMB. Create one for each in 
>> DSB and CMB ? They could share the same show/store routines, but could
>> be done with additional variable to indicate which attribute they are 
>> controlling. Like the other attributes, using dev_ext_attribute or such.
> 
> New approach below, please help review to see if it is acceptable?
> 
> #define tpdm_patt_enable_ts_rw(name, mem)                \
>      (&((struct tpdm_dataset_attribute[]) {            \
>         {                                \
>          __ATTR(name, 0644, enable_ts_show,        \
>          enable_ts_store),        \
>          mem,                            \
>         }                                \
>      })[0].attr.attr)
> 
> 
> #define DSB_PATT_ENABLE_TS                    \
>          tpdm_patt_enable_ts_rw(enable_ts,        \
>          DSB_PATT)
> 
> 
> #define CMB_PATT_ENABLE_TS                    \
>          tpdm_patt_enable_ts_rw(enable_ts,        \
>          CMB_PATT)
> 
> 
> static ssize_t enable_ts_show(struct device *dev,
>                    struct device_attribute *attr,
>                    char *buf)
> {
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
>      ssize_t size = 0;
> 
>      if (tpdm_attr->mem == DSB_PATT) {
>          size = sysfs_emit(buf, "%u\n",
>                   (unsigned int)drvdata->dsb->patt_ts);
>      } else if (tpdm_attr->mem == CMB_PATT) {
>          size = sysfs_emit(buf, "%u\n",
>                  (unsigned int)drvdata->cmb->patt_ts);
>      } else
>          return -EINVAL;
> 
>      return size;
> }
> 
> static ssize_t enable_ts_store(struct device *dev,
>                     struct device_attribute *attr,
>                     const char *buf,
>                     size_t size)
> {
>      struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>      struct tpdm_dataset_attribute *tpdm_attr =
>          container_of(attr, struct tpdm_dataset_attribute, attr);
>      unsigned long val;
> 
>      if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
>          return -EINVAL;
> 
>      spin_lock(&drvdata->spinlock);
>      if (tpdm_attr->mem == DSB_PATT) {
>          drvdata->dsb->patt_ts = !!val;
>      } else if (tpdm_attr->mem == CMB_PATT) {
>          drvdata->cmb->patt_ts = !!val;
>      } else
>          return -EINVAL;
>      spin_unlock(&drvdata->spinlock);
> 
>      return size;
> }
> 
> 

Yes, that is what I had in mind.

Thanks
Suzuki

> Best,
> 
> Tao
> 
>>
>>
>> Suzuki
>>