Messages in this thread Patch in this message | | | Date | Thu, 25 May 2023 16:12:53 +0800 | Subject | Re: [PATCH v4 03/11] coresight-tpdm: Initialize DSB subunit configuration | From | Tao Zhang <> |
| |
On 5/23/2023 9:42 PM, Suzuki K Poulose wrote: > On 27/04/2023 10:00, 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@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-tpdm.c | 60 >> +++++++++++++++++++++++++--- >> drivers/hwtracing/coresight/coresight-tpdm.h | 17 ++++++++ >> 2 files changed, 72 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index ba1867f..6f8a8ab 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -20,17 +20,51 @@ >> DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); >> +static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata) >> +{ >> + if (drvdata->datasets & TPDM_PIDR0_DS_DSB) { >> + memset(drvdata->dsb, 0, sizeof(struct dsb_dataset)); >> + >> + drvdata->dsb->trig_ts = true; >> + drvdata->dsb->trig_type = false; >> + } >> +} >> + >> +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; >> +} >> + > > Given this is not reused, we could simply inline it in the caller > to avoid creating a confusion, like other operations ? Sure, I will update it in the next patch series. > >> 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 > > minor nit: Please could you extend the existing comment than adding a > new multi-line comment ? Sure, I will update it in the next patch series. > >> + * 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 +144,24 @@ static const struct coresight_ops tpdm_cs_ops = { >> .source_ops = &tpdm_source_ops, >> }; >> -static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) >> +static int tpdm_datasets_setup(struct tpdm_drvdata *drvdata) >> { >> u32 pidr; >> - CS_UNLOCK(drvdata->base); >> /* 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); > > Why are we removing the CS_{UN,}LOCK here ?
CS_UNLOCK is used before writing data to Coresight registers. Here this function
doesn't need to write data to any registers, so I remove the CS_{UN,}LOCK here.
Best,
Tao
> > Rest looks OK to me. > > Suzuki > >> + >> + 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; >> + } >> + } >> + >> + return 0; >> } >> /* >> @@ -181,6 +224,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 +244,12 @@ static int tpdm_probe(struct amba_device *adev, >> const struct amba_id *id) >> drvdata->base = base; >> + ret = tpdm_datasets_setup(drvdata); >> + if (ret) >> + return ret; >> + >> + tpdm_reset_datasets(drvdata); >> + >> /* Set up coresight component description */ >> desc.name = coresight_alloc_device_name(&tpdm_devs, dev); >> if (!desc.name) >> @@ -216,7 +266,7 @@ 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); >> + >> /* Decrease pm refcount when probe is done.*/ >> pm_runtime_put(&adev->dev); >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h >> b/drivers/hwtracing/coresight/coresight-tpdm.h >> index 5438540..68f33bd 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >> @@ -11,8 +11,14 @@ >> /* DSB Subunit Registers */ >> #define TPDM_DSB_CR (0x780) >> +#define TPDM_DSB_TIER (0x784) >> + >> /* Enable bit for DSB subunit */ >> #define TPDM_DSB_CR_ENA BIT(0) >> +/* Enable bit for DSB subunit trigger type */ >> +#define TPDM_DSB_CR_TRIG_TYPE BIT(12) >> +/* Enable bit for DSB subunit trigger timestamp */ >> +#define TPDM_DSB_TIER_XTRIG_TSENAB BIT(1) >> /* TPDM integration test registers */ >> #define TPDM_ITATBCNTRL (0xEF0) >> @@ -41,6 +47,16 @@ >> #define TPDM_PIDR0_DS_DSB BIT(1) >> /** >> + * struct dsb_dataset - specifics associated to dsb dataset >> + * @trig_ts: Enable/Disable trigger timestamp. >> + * @trig_type: Enable/Disable trigger type. >> + */ >> +struct dsb_dataset { >> + bool trig_ts; >> + bool trig_type; >> +}; >> + >> +/** >> * struct tpdm_drvdata - specifics associated to an TPDM component >> * @base: memory mapped base address for this component. >> * @dev: The device entity associated to this component. >> @@ -57,6 +73,7 @@ struct tpdm_drvdata { >> spinlock_t spinlock; >> bool enable; >> unsigned long datasets; >> + struct dsb_dataset *dsb; >> }; >> #endif /* _CORESIGHT_CORESIGHT_TPDM_H */ > > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org
| |