On Mon, Dec 07, 2020 at 07:32:21PM +0800, Qi Liu wrote: > > > On 2020/12/7 19:27, Suzuki K Poulose wrote: > > On 12/7/20 11:21 AM, Qi Liu wrote: > >> > >> Hi Suzuki, > >> On 2020/12/7 18:38, Suzuki K Poulose wrote: > >>> On 12/7/20 2:08 AM, Qi Liu wrote: > >>>> Hi Mathieu, > >>>> > >>>> On 2020/12/5 2:55, Mathieu Poirier wrote: > >>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote: > >>>>>> The ETM device can't keep up with the core pipeline when cpu core > >>>>>> is at full speed. This may cause overflow within core and its ETM. > >>>>>> This is a common phenomenon on ETM devices. > >>>>>> > >>>>>> On HiSilicon Hip08 platform, a specific feature is added to set > >>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM > >>>>>> overflow. > >>>>>> > >>>>>> Signed-off-by: Qi Liu <[email protected]> > >>>>>> --- > >>>>>> Change since v1: > >>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON > >>>>>> to keep specific feature off platforms which don't use it. > >>>>>> Change since v2: > >>>>>> - remove some unused variable. > >>>>>> Change since v3: > >>>>>> - use read/write_sysreg_s() to access register. > >>>>>> Change since v4: > >>>>>> - rename the call back function to a more generic name, and fix some > >>>>>> compile warnings. > >>>>>> > >>>>>> drivers/hwtracing/coresight/Kconfig | 9 +++ > >>>>>> drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 > >>>>>> ++++++++++++++++++++++ > >>>>>> drivers/hwtracing/coresight/coresight-etm4x.h | 8 ++ > >>>>>> 3 files changed, 105 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig > >>>>>> b/drivers/hwtracing/coresight/Kconfig > >>>>>> index c119824..1cc3601 100644 > >>>>>> --- a/drivers/hwtracing/coresight/Kconfig > >>>>>> +++ b/drivers/hwtracing/coresight/Kconfig > >>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X > >>>>>> To compile this driver as a module, choose M here: the > >>>>>> module will be called coresight-etm4x. > >>>>>> > >>>>>> +config ETM4X_IMPDEF_FEATURE > >>>>>> + bool "Control overflow impdef support in CoreSight ETM 4.x driver > >>>>>> " > >>>>>> + depends on CORESIGHT_SOURCE_ETM4X > >>>>>> + help > >>>>>> + This control provides overflow implement define for CoreSight > >>>>>> + ETM 4.x tracer module which could not reduce commit race > >>>>>> + automatically, and could avoid overflow within ETM tracer module > >>>>>> + and its cpu core. > >>>>>> + > >>>>>> config CORESIGHT_STM > >>>>>> tristate "CoreSight System Trace Macrocell driver" > >>>>>> depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || > >>>>>> ARM64 > >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c > >>>>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c > >>>>>> index abd706b..fcee27a 100644 > >>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > >>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > >>>>>> @@ -3,6 +3,7 @@ > >>>>>> * Copyright (c) 2014, The Linux Foundation. All rights reserved. > >>>>>> */ > >>>>>> > >>>>>> +#include <linux/bitops.h> > >>>>>> #include <linux/kernel.h> > >>>>>> #include <linux/moduleparam.h> > >>>>>> #include <linux/init.h> > >>>>>> @@ -28,7 +29,9 @@ > >>>>>> #include <linux/perf_event.h> > >>>>>> #include <linux/pm_runtime.h> > >>>>>> #include <linux/property.h> > >>>>>> + > >>>>>> #include <asm/sections.h> > >>>>>> +#include <asm/sysreg.h> > >>>>>> #include <asm/local.h> > >>>>>> #include <asm/virt.h> > >>>>>> > >>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg { > >>>>>> int rc; > >>>>>> }; > >>>>>> > >>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE > >>>>>> + > >>>>>> +#define HISI_HIP08_AMBA_ID 0x000b6d01 > >>>>>> +#define ETM4_AMBA_MASK 0xfffff > >>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR 0x3000 > >>>>> > >>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() > >>>>> only bit 12 > >>>>> is set - is this intentional? What is bit 13 for? > >>>>> > >>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is > >>>> at full speed, > >>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 > >>>> means speed is > >>>> reduced to minimum value. So bit 12 and 13 should be cleared together in > >>>> etm4_hisi_config_core_commit(). > >>> > >>> Please could you document this in the function. > >>> > >> of course, thanks. > >>>> > >>>> Qi > >>>> > >>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT 12 > >>>>>> +#define HISI_HIP08_CORE_COMMIT_REG sys_reg(3, 1, 15, 2, 5) > >>>>>> + > >>>>>> +struct etm4_arch_features { > >>>>>> + void (*arch_callback)(bool enable); > >>>>>> +}; > >>>>>> + > >>>>>> +static bool etm4_hisi_match_pid(unsigned int id) > >>>>>> +{ > >>>>>> + return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID; > >>>>>> +} > >>>>>> + > >>>>>> +static void etm4_hisi_config_core_commit(bool enable) > >>>>>> +{ > >>>>>> + u64 val; > >>>>>> + > >>>>>> + val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG); > >>>>>> + val &= ~HISI_HIP08_CORE_COMMIT_CLEAR; > >>>>>> + val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT; > >>> > >>> I would use the explicitly masked values when you update > >>> a register. > >>> > >> ok, how about changing these code to this: > >> val &= ~GENMASK(12, 13); > > > > I would do : > > > > // Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK > > // above. > > #define HISI_HIP08_CORE_COMMIT_MASK 0x3000 > > > > #define HISI_HIP08_CORE_COMMIT_FULL 0b00 > > #define HISI_HIP08_CORE_COMMIT_LVL_1 0b01 > > > > > > u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : > > HISI_HIP08_CORE_COMMIT_FULL; > > > > ... > > > > val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT; > > > > .. > > > > > > Suzuki > > > > . > ok, I'll send a new version. :) >
Please do so by tomorrow morning (North America time) if you want to see this going in the v5.11 merge window. Otherwise it will be another 3 months. > Thanks > Qi > > > > > _______________________________________________ > linux-arm-kernel mailing list > [email protected] > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
