Messages in this thread | | | Subject | Re: [PATCH v2] coresight: etm4x: Handle accesses to TRCSTALLCTLR | From | Suzuki K Poulose <> | Date | Wed, 27 Jan 2021 18:29:59 +0000 |
| |
On 1/27/21 5:43 PM, Mathieu Poirier wrote: > Good day, > > On Wed, Jan 27, 2021 at 12:00:32PM +0000, Suzuki K Poulose wrote: >> TRCSTALLCTLR register is only implemented if >> >> TRCIDR3.STALLCTL == 0b1 >> >> Make sure the driver touches the register only it is implemented. >> >> Cc: stable@vger.kernel.org >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Cc: Leo Yan <leo.yan@linaro.org> >> Cc: Mike Leach <mike.leach@linaro.org> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> Changes since v1: >> - No change to the patch, fixed the stable email address and >> added usual reviewers. >> --- >> drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 ++++++--- >> drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 3 +++ >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index b40e3c2bf818..814b49dae0c7 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -367,7 +367,8 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) >> etm4x_relaxed_write32(csa, 0x0, TRCAUXCTLR); >> etm4x_relaxed_write32(csa, config->eventctrl0, TRCEVENTCTL0R); >> etm4x_relaxed_write32(csa, config->eventctrl1, TRCEVENTCTL1R); >> - etm4x_relaxed_write32(csa, config->stall_ctrl, TRCSTALLCTLR); >> + if (drvdata->stallctl) >> + etm4x_relaxed_write32(csa, config->stall_ctrl, TRCSTALLCTLR); >> etm4x_relaxed_write32(csa, config->ts_ctrl, TRCTSCTLR); >> etm4x_relaxed_write32(csa, config->syncfreq, TRCSYNCPR); >> etm4x_relaxed_write32(csa, config->ccctlr, TRCCCCTLR); >> @@ -1545,7 +1546,8 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata) >> state->trcauxctlr = etm4x_read32(csa, TRCAUXCTLR); >> state->trceventctl0r = etm4x_read32(csa, TRCEVENTCTL0R); >> state->trceventctl1r = etm4x_read32(csa, TRCEVENTCTL1R); >> - state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR); >> + if (drvdata->stallctl) >> + state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR); >> state->trctsctlr = etm4x_read32(csa, TRCTSCTLR); >> state->trcsyncpr = etm4x_read32(csa, TRCSYNCPR); >> state->trcccctlr = etm4x_read32(csa, TRCCCCTLR); >> @@ -1657,7 +1659,8 @@ static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) >> etm4x_relaxed_write32(csa, state->trcauxctlr, TRCAUXCTLR); >> etm4x_relaxed_write32(csa, state->trceventctl0r, TRCEVENTCTL0R); >> etm4x_relaxed_write32(csa, state->trceventctl1r, TRCEVENTCTL1R); >> - etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR); >> + if (drvdata->stallctl) >> + etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR); >> etm4x_relaxed_write32(csa, state->trctsctlr, TRCTSCTLR); >> etm4x_relaxed_write32(csa, state->trcsyncpr, TRCSYNCPR); >> etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR); >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> index 1c490bcef3ad..cd9249fbf913 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> @@ -296,6 +296,9 @@ static ssize_t mode_store(struct device *dev, >> if (kstrtoul(buf, 16, &val)) >> return -EINVAL; >> >> + if ((val & ETM_MODE_ISTALL_EN) && !drvdata->stallctl) >> + return -EINVAL; >> + > > We have two choices here: > > 1) Follow what is already done in this function for implementation define > options like ETM_MODE_BB, ETMv4_MODE_CTXID, ETM_MODE_RETURNSTACK and others. In > that case we would have: > > /* bit[8], Instruction stall bit */ > if ((config->mode & ETM_MODE_ISTALL_EN) && drvdata->stallctl == true)) > config->stall_ctrl |= BIT(8); > else > config->stall_ctrl &= ~BIT(8); > > 2) Return -EINVAL when something is not supported, like you have above. In that > case we'd have to enact the same behavior for all the options, which has the > potential of breaking user space.
I did think about this and but now I agree 1 is better for now. I will respin.
Cheers Suzuki
| |