Re: [PATCH v3 6/8] coresight-tpdm: Add timestamp control register support for the CMB
From: Suzuki K Poulose
Date: Fri Jan 12 2024 - 04:30:55 EST
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