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 <liuqi...@huawei.com>
>>>> ---
>>>> 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);
Thanks
Qi
> With the above:
>
> Reviewed-by: Suzuki K Poulose <suzuki.poul...@arm.com>
>
> .
>
