Hi Suzuki,
On 3/23/2023 10:23 PM, Suzuki K Poulose wrote:
On 23/03/2023 06:04, Tao Zhang wrote:
DSB is used for monitoring “events”. Events are something that
occurs at some point in time. It could be a state decode, the
act of writing/reading a particular address, a FIFO being empty,
etc. This decoding of the event desired is done outside TPDM.
DSB subunit need to be configured in enablement and disablement.
A struct that specifics associated to dsb dataset is needed. It
saves the configuration and parameters of the dsb datasets. This
change is to add this struct and initialize the configuration of
DSB subunit.
Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
---
drivers/hwtracing/coresight/coresight-tpdm.c | 58
+++++++++++++++++++++++++---
drivers/hwtracing/coresight/coresight-tpdm.h | 17 ++++++++
2 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
b/drivers/hwtracing/coresight/coresight-tpdm.c
index f4854af..5e1e2ba 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -20,17 +20,59 @@
DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
+static int tpdm_init_datasets(struct tpdm_drvdata *drvdata)
+{
+ if (drvdata->datasets & TPDM_PIDR0_DS_DSB) {
+ if (!drvdata->dsb) {
+ drvdata->dsb = devm_kzalloc(drvdata->dev,
+ sizeof(*drvdata->dsb), GFP_KERNEL);
+ if (!drvdata->dsb)
+ return -ENOMEM;
Please do not club init/allocation of datasets to "resetting" the
datasets. Why don't we move the allocation to tpmd_datasets_setup() ?
And this function could simply be called :
tpdm_reset_datasets()
and can be called from the tpdm_datasets_setup() too.
I will update this in the next patch series.
+ } else
+ memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
+
+ drvdata->dsb->trig_ts = true;
+ drvdata->dsb->trig_type = false;
+ }
+
+ return 0;
+}
+
+static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val)
+{
+ if (drvdata->dsb->trig_type)
+ *val |= TPDM_DSB_CR_TRIG_TYPE;
+ else
+ *val &= ~TPDM_DSB_CR_TRIG_TYPE;
+}
+
Do we really need a function for this ? How is it different from
trig_ts ?
"trig_type" is for setting trigger type, and "trig_ts" is for setting
trigger
timestamp. These two variables are configured in two different registers.
Do you mean we don't need to wrap it as a function here?
static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
{
u32 val;
- /* Set the enable bit of DSB control register to 1 */
+ val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
+ /* Set trigger timestamp */
+ if (drvdata->dsb->trig_ts)
+ val |= TPDM_DSB_TIER_XTRIG_TSENAB;
+ else
+ val &= ~TPDM_DSB_TIER_XTRIG_TSENAB;
+ writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
+
val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
+ /* Set trigger type */
+ set_trigger_type(drvdata, &val);
+ /* Set the enable bit of DSB control register to 1 */
val |= TPDM_DSB_CR_ENA;
writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
}
/* TPDM enable operations */
+/* The TPDM or Monitor serves as data collection component for various
+ * dataset types. It covers Basic Counts(BC), Tenure Counts(TC),
+ * Continuous Multi-Bit(CMB), Multi-lane CMB(MCMB) and Discrete Single
+ * Bit(DSB). This function will initialize the configuration according
+ * to the dataset type supported by the TPDM.
+ */
static void __tpdm_enable(struct tpdm_drvdata *drvdata)
{
CS_UNLOCK(drvdata->base);
@@ -110,15 +152,13 @@ static const struct coresight_ops tpdm_cs_ops = {
.source_ops = &tpdm_source_ops,
};
-static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
+static void tpdm_datasets_setup(struct tpdm_drvdata *drvdata)
{
u32 pidr;
- CS_UNLOCK(drvdata->base);
Why is this removed ?
We only need to read the register here, don't need to unlock the
registers here, right?
/* Get the datasets present on the TPDM. */
pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
- CS_LOCK(drvdata->base);
}
/*
@@ -181,6 +221,7 @@ static int tpdm_probe(struct amba_device *adev,
const struct amba_id *id)
struct coresight_platform_data *pdata;
struct tpdm_drvdata *drvdata;
struct coresight_desc desc = { 0 };
+ int ret;
pdata = coresight_get_platform_data(dev);
if (IS_ERR(pdata))
@@ -200,6 +241,8 @@ static int tpdm_probe(struct amba_device *adev,
const struct amba_id *id)
drvdata->base = base;
+ tpdm_datasets_setup(drvdata);
+
/* Set up coresight component description */
desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
if (!desc.name)
@@ -216,7 +259,12 @@ static int tpdm_probe(struct amba_device *adev,
const struct amba_id *id)
return PTR_ERR(drvdata->csdev);
spin_lock_init(&drvdata->spinlock);
- tpdm_init_default_data(drvdata);
+ ret = tpdm_init_datasets(drvdata);
Couldn't this be done before the coresight_register() ? As such
I don't see any dependency on having a csdev ?
I will update this in the next patch series.
Tao
Suzuki
