Messages in this thread | | | From | Mike Leach <> | Date | Tue, 12 Apr 2022 11:15:15 +0100 | Subject | Re: [PATCH v3 09/15] coresight: etm4x: Cleanup TRCVICTLR register accesses |
| |
On Mon, 28 Mar 2022 at 11:41, James Clark <james.clark@arm.com> wrote: > > > > On 23/03/2022 15:59, Mathieu Poirier wrote: > > Hi James, > > > > On Fri, Mar 04, 2022 at 05:19:06PM +0000, James Clark wrote: > >> This is a no-op change for style and consistency and has no effect on > >> the binary output by the compiler. In sysreg.h fields are defined as > >> the register name followed by the field name and then _MASK. This > >> allows for grepping for fields by name rather than using magic numbers. > >> > >> Signed-off-by: James Clark <james.clark@arm.com> > >> --- > >> .../coresight/coresight-etm4x-core.c | 10 +++--- > >> .../coresight/coresight-etm4x-sysfs.c | 36 +++++++++---------- > >> drivers/hwtracing/coresight/coresight-etm4x.h | 22 +++++------- > >> 3 files changed, 31 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > >> index 445e2057d5ed..88353f8ba414 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > >> @@ -1206,7 +1206,7 @@ static void etm4_init_arch_data(void *info) > >> > >> static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config) > >> { > >> - return etm4_get_access_type(config) << TRCVICTLR_EXLEVEL_SHIFT; > >> + return etm4_get_access_type(config) << __bf_shf(TRCVICTLR_EXLEVEL_MASK); > >> } > >> > >> /* Set ELx trace filter access in the TRCVICTLR register */ > >> @@ -1232,7 +1232,7 @@ static void etm4_set_default_config(struct etmv4_config *config) > >> config->ts_ctrl = 0x0; > >> > >> /* TRCVICTLR::EVENT = 0x01, select the always on logic */ > >> - config->vinst_ctrl = BIT(0); > >> + config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01); > >> > >> /* TRCVICTLR::EXLEVEL_NS:EXLEVELS: Set kernel / user filtering */ > >> etm4_set_victlr_access(config); > >> @@ -1341,7 +1341,7 @@ static void etm4_set_default_filter(struct etmv4_config *config) > >> * TRCVICTLR::SSSTATUS == 1, the start-stop logic is > >> * in the started state > >> */ > >> - config->vinst_ctrl |= BIT(9); > >> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS; > >> config->mode |= ETM_MODE_VIEWINST_STARTSTOP; > >> > >> /* No start-stop filtering for ViewInst */ > >> @@ -1445,7 +1445,7 @@ static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, > >> * TRCVICTLR::SSSTATUS == 1, the start-stop logic is > >> * in the started state > >> */ > >> - config->vinst_ctrl |= BIT(9); > >> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS; > >> > >> /* No start-stop filtering for ViewInst */ > >> config->vissctlr = 0x0; > >> @@ -1473,7 +1473,7 @@ static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, > >> * etm4_disable_perf(). > >> */ > >> if (filters->ssstatus) > >> - config->vinst_ctrl |= BIT(9); > >> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS; > >> > >> /* No include/exclude filtering for ViewInst */ > >> config->viiectlr = 0x0; > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> index cd24590ea38a..b3b1b92909cc 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> @@ -206,11 +206,11 @@ static ssize_t reset_store(struct device *dev, > >> * started state. ARM recommends start-stop logic is set before > >> * each trace run. > >> */ > >> - config->vinst_ctrl = BIT(0); > >> + config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01); > >> if (drvdata->nr_addr_cmp > 0) { > >> config->mode |= ETM_MODE_VIEWINST_STARTSTOP; > >> /* SSSTATUS, bit[9] */ > >> - config->vinst_ctrl |= BIT(9); > >> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS; > >> } > >> > >> /* No address range filtering for ViewInst */ > >> @@ -416,22 +416,22 @@ static ssize_t mode_store(struct device *dev, > >> > >> /* bit[9] Start/stop logic control bit */ > >> if (config->mode & ETM_MODE_VIEWINST_STARTSTOP) > >> - config->vinst_ctrl |= BIT(9); > >> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS; > >> else > >> - config->vinst_ctrl &= ~BIT(9); > >> + config->vinst_ctrl &= ~TRCVICTLR_SSSTATUS; > >> > >> /* bit[10], Whether a trace unit must trace a Reset exception */ > >> if (config->mode & ETM_MODE_TRACE_RESET) > >> - config->vinst_ctrl |= BIT(10); > >> + config->vinst_ctrl |= TRCVICTLR_TRCRESET; > >> else > >> - config->vinst_ctrl &= ~BIT(10); > >> + config->vinst_ctrl &= ~TRCVICTLR_TRCRESET; > >> > >> /* bit[11], Whether a trace unit must trace a system error exception */ > >> if ((config->mode & ETM_MODE_TRACE_ERR) && > >> (drvdata->trc_error == true)) > >> - config->vinst_ctrl |= BIT(11); > >> + config->vinst_ctrl |= TRCVICTLR_TRCERR; > >> else > >> - config->vinst_ctrl &= ~BIT(11); > >> + config->vinst_ctrl &= ~TRCVICTLR_TRCERR; > >> > >> if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER)) > >> etm4_config_trace_mode(config); > >> @@ -723,7 +723,7 @@ static ssize_t event_vinst_show(struct device *dev, > >> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > >> struct etmv4_config *config = &drvdata->config; > >> > >> - val = config->vinst_ctrl & ETMv4_EVENT_MASK; > >> + val = FIELD_GET(TRCVICTLR_EVENT_MASK, config->vinst_ctrl); > >> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > >> } > >> > >> @@ -739,9 +739,9 @@ static ssize_t event_vinst_store(struct device *dev, > >> return -EINVAL; > >> > >> spin_lock(&drvdata->spinlock); > >> - val &= ETMv4_EVENT_MASK; > >> - config->vinst_ctrl &= ~ETMv4_EVENT_MASK; > >> - config->vinst_ctrl |= val; > >> + val &= TRCVICTLR_EVENT_MASK >> __bf_shf(TRCVICTLR_EVENT_MASK); > > > > Not sure why the right-shifting operation is needed since the mask starts at bit > > 0. Please consider fixing _if_ you end up respinning this. > > Yes you're right it doesn't need shifting. I suppose I did it that way so that when > reading the code it's obvious that 'val' is a value starting at bit 0, rather than > the original state of the register shifted to some arbitrary position. > > Someone reading the code wouldn't know that TRCVICTLR_EVENT_MASK starts at 0, > so without the explicit shift they might think excluding it was a mistake. It also > makes it consistent with other uses of masks that don't start at 0. > > James > > > > > Thanks, > > Mathieu > > > >> + config->vinst_ctrl &= ~TRCVICTLR_EVENT_MASK; > >> + config->vinst_ctrl |= FIELD_PREP(TRCVICTLR_EVENT_MASK, val); > >> spin_unlock(&drvdata->spinlock); > >> return size; > >> } > >> @@ -755,7 +755,7 @@ static ssize_t s_exlevel_vinst_show(struct device *dev, > >> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > >> struct etmv4_config *config = &drvdata->config; > >> > >> - val = (config->vinst_ctrl & TRCVICTLR_EXLEVEL_S_MASK) >> TRCVICTLR_EXLEVEL_S_SHIFT; > >> + val = FIELD_GET(TRCVICTLR_EXLEVEL_S_MASK, config->vinst_ctrl); > >> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > >> } > >> > >> @@ -772,10 +772,10 @@ static ssize_t s_exlevel_vinst_store(struct device *dev, > >> > >> spin_lock(&drvdata->spinlock); > >> /* clear all EXLEVEL_S bits */ > >> - config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_S_MASK); > >> + config->vinst_ctrl &= ~TRCVICTLR_EXLEVEL_S_MASK; > >> /* enable instruction tracing for corresponding exception level */ > >> val &= drvdata->s_ex_level; > >> - config->vinst_ctrl |= (val << TRCVICTLR_EXLEVEL_S_SHIFT); > >> + config->vinst_ctrl |= val << __bf_shf(TRCVICTLR_EXLEVEL_S_MASK); > >> spin_unlock(&drvdata->spinlock); > >> return size; > >> } > >> @@ -790,7 +790,7 @@ static ssize_t ns_exlevel_vinst_show(struct device *dev, > >> struct etmv4_config *config = &drvdata->config; > >> > >> /* EXLEVEL_NS, bits[23:20] */ > >> - val = (config->vinst_ctrl & TRCVICTLR_EXLEVEL_NS_MASK) >> TRCVICTLR_EXLEVEL_NS_SHIFT; > >> + val = FIELD_GET(TRCVICTLR_EXLEVEL_NS_MASK, config->vinst_ctrl); > >> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > >> } > >> > >> @@ -807,10 +807,10 @@ static ssize_t ns_exlevel_vinst_store(struct device *dev, > >> > >> spin_lock(&drvdata->spinlock); > >> /* clear EXLEVEL_NS bits */ > >> - config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_NS_MASK); > >> + config->vinst_ctrl &= ~TRCVICTLR_EXLEVEL_NS_MASK; > >> /* enable instruction tracing for corresponding exception level */ > >> val &= drvdata->ns_ex_level; > >> - config->vinst_ctrl |= (val << TRCVICTLR_EXLEVEL_NS_SHIFT); > >> + config->vinst_ctrl |= val << __bf_shf(TRCVICTLR_EXLEVEL_NS_MASK); > >> spin_unlock(&drvdata->spinlock); > >> return size; > >> } > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h > >> index 36934056a5dc..9cacc38b1890 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x.h > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h > >> @@ -200,6 +200,14 @@ > >> #define TRCSTALLCTLR_INSTPRIORITY BIT(10) > >> #define TRCSTALLCTLR_NOOVERFLOW BIT(13) > >> > >> +#define TRCVICTLR_EVENT_MASK GENMASK(7, 0) > >> +#define TRCVICTLR_SSSTATUS BIT(9) > >> +#define TRCVICTLR_TRCRESET BIT(10) > >> +#define TRCVICTLR_TRCERR BIT(11) > >> +#define TRCVICTLR_EXLEVEL_MASK GENMASK(22, 16) > >> +#define TRCVICTLR_EXLEVEL_S_MASK GENMASK(19, 16) > >> +#define TRCVICTLR_EXLEVEL_NS_MASK GENMASK(22, 20) > >> + > >> /* > >> * System instructions to access ETM registers. > >> * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions > >> @@ -700,23 +708,9 @@ > >> #define ETM_EXLEVEL_NS_OS BIT(5) /* NonSecure EL1 */ > >> #define ETM_EXLEVEL_NS_HYP BIT(6) /* NonSecure EL2 */ > >> > >> -#define ETM_EXLEVEL_MASK (GENMASK(6, 0)) > >> -#define ETM_EXLEVEL_S_MASK (GENMASK(3, 0)) > >> -#define ETM_EXLEVEL_NS_MASK (GENMASK(6, 4)) > >> - > >> /* access level controls in TRCACATRn */ > >> #define TRCACATR_EXLEVEL_SHIFT 8 > >> > >> -/* access level control in TRCVICTLR */ > >> -#define TRCVICTLR_EXLEVEL_SHIFT 16 > >> -#define TRCVICTLR_EXLEVEL_S_SHIFT 16 > >> -#define TRCVICTLR_EXLEVEL_NS_SHIFT 20 > >> - > >> -/* secure / non secure masks - TRCVICTLR, IDR3 */ > >> -#define TRCVICTLR_EXLEVEL_MASK (ETM_EXLEVEL_MASK << TRCVICTLR_EXLEVEL_SHIFT) > >> -#define TRCVICTLR_EXLEVEL_S_MASK (ETM_EXLEVEL_S_MASK << TRCVICTLR_EXLEVEL_SHIFT) > >> -#define TRCVICTLR_EXLEVEL_NS_MASK (ETM_EXLEVEL_NS_MASK << TRCVICTLR_EXLEVEL_SHIFT) > >> - > >> #define ETM_TRCIDR1_ARCH_MAJOR_SHIFT 8 > >> #define ETM_TRCIDR1_ARCH_MAJOR_MASK (0xfU << ETM_TRCIDR1_ARCH_MAJOR_SHIFT) > >> #define ETM_TRCIDR1_ARCH_MAJOR(x) \ > >> -- > >> 2.28.0 > >>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
| |