Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220304171913.2292458-1-james.clark@arm.com> <20220304171913.2292458-7-james.clark@arm.com>
In-Reply-To: <20220304171913.2292458-7-james.clark@arm.com>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Tue, 12 Apr 2022 09:49:04 +0100
Message-ID: <CAJ9a7VjwfoZg6HZfs7obzx2-AGMs0OL-2roTXjez04ik2dU4kA@mail.gmail.com>
Subject: Re: [PATCH v3 06/15] coresight: etm4x: Cleanup TRCCONFIGR register accesses
To:     James Clark <James.Clark@arm.com>
Cc:     suzuki.poulose@arm.com, coresight@lists.linaro.org,
        Anshuman.Khandual@arm.com, mathieu.poirier@linaro.org,
        leo.yan@linaro.com, Leo Yan <leo.yan@linaro.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 4 Mar 2022 at 17:19, James Clark <james.clark@arm.com> 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          | 12 ++---
>  .../coresight/coresight-etm4x-sysfs.c         | 46 +++++++++----------
>  drivers/hwtracing/coresight/coresight-etm4x.h | 16 +++++++
>  3 files changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 3f4263117570..445e2057d5ed 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -633,7 +633,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>
>         /* Go from generic option to ETMv4 specifics */
>         if (attr->config & BIT(ETM_OPT_CYCACC)) {
> -               config->cfg |= BIT(4);
> +               config->cfg |= TRCCONFIGR_CCI;
>                 /* TRM: Must program this for cycacc to work */
>                 config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT;
>         }
> @@ -653,14 +653,14 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>                         goto out;
>
>                 /* bit[11], Global timestamp tracing bit */
> -               config->cfg |= BIT(11);
> +               config->cfg |= TRCCONFIGR_TS;
>         }
>
>         /* Only trace contextID when runs in root PID namespace */
>         if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
>             task_is_in_init_pid_ns(current))
>                 /* bit[6], Context ID tracing bit */
> -               config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
> +               config->cfg |= TRCCONFIGR_CID;
>
>         /*
>          * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID
> @@ -672,17 +672,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>                         ret = -EINVAL;
>                         goto out;
>                 }
> -
>                 /* Only trace virtual contextID when runs in root PID namespace */
>                 if (task_is_in_init_pid_ns(current))
> -                       config->cfg |= BIT(ETM4_CFG_BIT_VMID) |
> -                                      BIT(ETM4_CFG_BIT_VMID_OPT);
> +                       config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT;
>         }
>
>         /* return stack - enable if selected and supported */
>         if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
>                 /* bit[12], Return stack enable bit */
> -               config->cfg |= BIT(12);
> +               config->cfg |= TRCCONFIGR_RS;
>
>         /*
>          * Set any selected configuration and preset.
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index 21687cc1e4e2..53f84da3fe44 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -180,12 +180,12 @@ static ssize_t reset_store(struct device *dev,
>
>         /* Disable data tracing: do not trace load and store data transfers */
>         config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
> -       config->cfg &= ~(BIT(1) | BIT(2));
> +       config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
>
>         /* Disable data value and data address tracing */
>         config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
>                            ETM_MODE_DATA_TRACE_VAL);
> -       config->cfg &= ~(BIT(16) | BIT(17));
> +       config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
>
>         /* Disable all events tracing */
>         config->eventctrl0 = 0x0;
> @@ -304,82 +304,82 @@ static ssize_t mode_store(struct device *dev,
>
>         if (drvdata->instrp0 == true) {
>                 /* start by clearing instruction P0 field */
> -               config->cfg  &= ~(BIT(1) | BIT(2));
> +               config->cfg  &= ~TRCCONFIGR_INSTP0_LOAD_STORE;
>                 if (config->mode & ETM_MODE_LOAD)
>                         /* 0b01 Trace load instructions as P0 instructions */
> -                       config->cfg  |= BIT(1);
> +                       config->cfg  |= TRCCONFIGR_INSTP0_LOAD;
>                 if (config->mode & ETM_MODE_STORE)
>                         /* 0b10 Trace store instructions as P0 instructions */
> -                       config->cfg  |= BIT(2);
> +                       config->cfg  |= TRCCONFIGR_INSTP0_STORE;
>                 if (config->mode & ETM_MODE_LOAD_STORE)
>                         /*
>                          * 0b11 Trace load and store instructions
>                          * as P0 instructions
>                          */
> -                       config->cfg  |= BIT(1) | BIT(2);
> +                       config->cfg  |= TRCCONFIGR_INSTP0_LOAD_STORE;
>         }
>
>         /* bit[3], Branch broadcast mode */
>         if ((config->mode & ETM_MODE_BB) && (drvdata->trcbb == true))
> -               config->cfg |= BIT(3);
> +               config->cfg |= TRCCONFIGR_BB;
>         else
> -               config->cfg &= ~BIT(3);
> +               config->cfg &= ~TRCCONFIGR_BB;
>
>         /* bit[4], Cycle counting instruction trace bit */
>         if ((config->mode & ETMv4_MODE_CYCACC) &&
>                 (drvdata->trccci == true))
> -               config->cfg |= BIT(4);
> +               config->cfg |= TRCCONFIGR_CCI;
>         else
> -               config->cfg &= ~BIT(4);
> +               config->cfg &= ~TRCCONFIGR_CCI;
>
>         /* bit[6], Context ID tracing bit */
>         if ((config->mode & ETMv4_MODE_CTXID) && (drvdata->ctxid_size))
> -               config->cfg |= BIT(6);
> +               config->cfg |= TRCCONFIGR_CID;
>         else
> -               config->cfg &= ~BIT(6);
> +               config->cfg &= ~TRCCONFIGR_CID;
>
>         if ((config->mode & ETM_MODE_VMID) && (drvdata->vmid_size))
> -               config->cfg |= BIT(7);
> +               config->cfg |= TRCCONFIGR_VMID;
>         else
> -               config->cfg &= ~BIT(7);
> +               config->cfg &= ~TRCCONFIGR_VMID;
>
>         /* bits[10:8], Conditional instruction tracing bit */
>         mode = ETM_MODE_COND(config->mode);
>         if (drvdata->trccond == true) {
> -               config->cfg &= ~(BIT(8) | BIT(9) | BIT(10));
> -               config->cfg |= mode << 8;
> +               config->cfg &= ~TRCCONFIGR_COND_MASK;
> +               config->cfg |= mode << __bf_shf(TRCCONFIGR_COND_MASK);
>         }
>
>         /* bit[11], Global timestamp tracing bit */
>         if ((config->mode & ETMv4_MODE_TIMESTAMP) && (drvdata->ts_size))
> -               config->cfg |= BIT(11);
> +               config->cfg |= TRCCONFIGR_TS;
>         else
> -               config->cfg &= ~BIT(11);
> +               config->cfg &= ~TRCCONFIGR_TS;
>
>         /* bit[12], Return stack enable bit */
>         if ((config->mode & ETM_MODE_RETURNSTACK) &&
>                                         (drvdata->retstack == true))
> -               config->cfg |= BIT(12);
> +               config->cfg |= TRCCONFIGR_RS;
>         else
> -               config->cfg &= ~BIT(12);
> +               config->cfg &= ~TRCCONFIGR_RS;
>
>         /* bits[14:13], Q element enable field */
>         mode = ETM_MODE_QELEM(config->mode);
>         /* start by clearing QE bits */
> -       config->cfg &= ~(BIT(13) | BIT(14));
> +       config->cfg &= ~(TRCCONFIGR_QE_W_COUNTS | TRCCONFIGR_QE_WO_COUNTS);
>         /*
>          * if supported, Q elements with instruction counts are enabled.
>          * Always set the low bit for any requested mode. Valid combos are
>          * 0b00, 0b01 and 0b11.
>          */
>         if (mode && drvdata->q_support)
> -               config->cfg |= BIT(13);
> +               config->cfg |= TRCCONFIGR_QE_W_COUNTS;
>         /*
>          * if supported, Q elements with and without instruction
>          * counts are enabled
>          */
>         if ((mode & BIT(1)) && (drvdata->q_support & BIT(1)))
> -               config->cfg |= BIT(14);
> +               config->cfg |= TRCCONFIGR_QE_WO_COUNTS;
>
>         /* bit[11], AMBA Trace Bus (ATB) trigger enable bit */
>         if ((config->mode & ETM_MODE_ATB_TRIGGER) &&
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 3b604cde668b..4c8d7be3c159 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -172,6 +172,22 @@
>  #define TRCIDR5_NUMSEQSTATE_MASK               GENMASK(27, 25)
>  #define TRCIDR5_NUMCNTR_MASK                   GENMASK(30, 28)
>
> +#define TRCCONFIGR_INSTP0_LOAD                 BIT(1)
> +#define TRCCONFIGR_INSTP0_STORE                        BIT(2)
> +#define TRCCONFIGR_INSTP0_LOAD_STORE           (TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE)
> +#define TRCCONFIGR_BB                          BIT(3)
> +#define TRCCONFIGR_CCI                         BIT(4)
> +#define TRCCONFIGR_CID                         BIT(6)
> +#define TRCCONFIGR_VMID                                BIT(7)
> +#define TRCCONFIGR_COND_MASK                   GENMASK(10, 8)
> +#define TRCCONFIGR_TS                          BIT(11)
> +#define TRCCONFIGR_RS                          BIT(12)
> +#define TRCCONFIGR_QE_W_COUNTS                 BIT(13)
> +#define TRCCONFIGR_QE_WO_COUNTS                        BIT(14)
> +#define TRCCONFIGR_VMIDOPT                     BIT(15)
> +#define TRCCONFIGR_DA                          BIT(16)
> +#define TRCCONFIGR_DV                          BIT(17)
> +
>  /*
>   * System instructions to access ETM registers.
>   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> --
> 2.28.0
>


Reviewed-by: Mike Leach <mike.leach@linaro.org>

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK