linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Qi Liu <liuqi115@huawei.com>
To: Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: <mike.leach@linaro.org>, <coresight@lists.linaro.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linuxarm@huawei.com>
Subject: Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
Date: Mon, 7 Dec 2020 19:32:21 +0800	[thread overview]
Message-ID: <6078c05c-273d-a9cb-6dcb-68009260a581@huawei.com> (raw)
In-Reply-To: <d82f1200-c31f-1040-a630-57e132cabf1b@arm.com>



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 <liuqi115@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);
> 
> 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. :)

Thanks
Qi
> 


  reply	other threads:[~2020-12-07 11:33 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-26 13:34 [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM Qi Liu
2020-12-04 18:55 ` Mathieu Poirier
2020-12-07  2:08   ` Qi Liu
2020-12-07 10:38     ` Suzuki K Poulose
2020-12-07 11:21       ` Qi Liu
2020-12-07 11:27         ` Suzuki K Poulose
2020-12-07 11:32           ` Qi Liu [this message]
2020-12-07 16:00             ` Mathieu Poirier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6078c05c-273d-a9cb-6dcb-68009260a581@huawei.com \
    --to=liuqi115@huawei.com \
    --cc=coresight@lists.linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=mike.leach@linaro.org \
    --cc=suzuki.poulose@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).