From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suzuki K Poulose Subject: Re: [PATCH v5 6/9] coresight: add support for CPU debug module Date: Wed, 29 Mar 2017 10:07:07 +0100 Message-ID: <5584203e-cb19-a5d2-45b1-3e78d3482c55@arm.com> References: <1490466197-29163-1-git-send-email-leo.yan@linaro.org> <1490466197-29163-7-git-send-email-leo.yan@linaro.org> <31be033f-514e-e48a-3ba2-a5c5cd477548@arm.com> <20170329030735.GA23889@leoy-linaro> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170329030735.GA23889@leoy-linaro> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Leo Yan Cc: Jonathan Corbet , Rob Herring , Mark Rutland , Wei Xu , Catalin Marinas , Will Deacon , Andy Gross , David Brown , Michael Turquette , Stephen Boyd , Mathieu Poirier , Guodong Xu , John Stultz , linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, mike.leach-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, sudeep.holla-5wv7dgnIgG8@public.gmane.org List-Id: linux-arm-msm@vger.kernel.org On 29/03/17 04:07, Leo Yan wrote: > Hi Suzuki, > > On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote: >> On 25/03/17 18:23, Leo Yan wrote: > > [...] > >> Leo, >> >> Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the >> idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert >> in that area. Some minor comments below. > > Thanks a lot for quick reviewing :) > >>> Signed-off-by: Leo Yan >>> --- >>> drivers/hwtracing/coresight/Kconfig | 11 + >>> drivers/hwtracing/coresight/Makefile | 1 + >>> drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++ >>> 3 files changed, 716 insertions(+) >>> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c >>> >>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig >>> index 130cb21..18d7931 100644 >>> --- a/drivers/hwtracing/coresight/Kconfig >>> +++ b/drivers/hwtracing/coresight/Kconfig >>> @@ -89,4 +89,15 @@ config CORESIGHT_STM >>> logging useful software events or data coming from various entities >>> in the system, possibly running different OSs >>> >>> +config CORESIGHT_CPU_DEBUG >>> + tristate "CoreSight CPU Debug driver" >>> + depends on ARM || ARM64 >>> + depends on DEBUG_FS >>> + help >>> + This driver provides support for coresight debugging module. This >>> + is primarily used to dump sample-based profiling registers when >>> + system triggers panic, the driver will parse context registers so >>> + can quickly get to know program counter (PC), secure state, >>> + exception level, etc. >> >> May be we should mention/warn the user about the possible caveats of using >> this feature to help him make a better decision ? And / Or we should add a documentation >> for it. We have collected some real good information over the discussions and >> it is a good idea to capture it somewhere. > > Sure, I will add a documentation for this. > > [...] > >>> +static struct pm_qos_request debug_qos_req; >>> +static int idle_constraint = PM_QOS_DEFAULT_VALUE; >>> +module_param(idle_constraint, int, 0600); >>> +MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU " >>> + "idle states (default is -1, which means have no limiation " >>> + "to CPU idle states; 0 means disabling all idle states; user " >>> + "can choose other platform dependent values so can disable " >>> + "specific idle states for the platform)"); >> >> Correct me if I am wrong, >> >> All we want to do is disable the CPUIdle explicitly if the user knows that this >> could be a problem to use CPU debug on his platform. So, in effect, we should >> only be using idle_constraint = 0 or -1. >> >> In which case, we could make it easier for the user to tell us, either >> >> 0 - Don't do anything with CPUIdle (default) >> 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle. > > The reason for not using bool flag is: usually SoC may have many idle > states, so if user wants to partially enable some states then can set > the latency to constraint. > > But of course, we can change this to binary value as you suggested, > this means turn on of turn off all states. The only one reason to use > latency value is it is more friendly for hardware design, e.g. some > platforms can enable partial states to save power and avoid overheat > after using this driver. > > If you guys think this is a bit over design, I will follow up your > suggestion. I also have some replying in Mathieu's reviewing, please > help review as well. > >> than explaining the miscrosecond latency etc and make the appropriate calls underneath. >> something like (not necessarily the same name) : >> >> module_param(broken_with_cpuidle, bool, 0600); >> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on" >> " the platform. Non-zero value implies CPUIdle has to be" >> " explicitly disabled.",); > > [...] > >>> + /* >>> + * Unfortunately the CPU cannot be powered up, so return >>> + * back and later has no permission to access other >>> + * registers. For this case, should set 'idle_constraint' >>> + * to ensure CPU power domain is enabled! >>> + */ >>> + if (!(drvdata->edprsr & EDPRSR_PU)) { >>> + pr_err("%s: power up request for CPU%d failed\n", >>> + __func__, drvdata->cpu); >>> + goto out; >>> + } >>> + >>> +out_powered_up: >>> + debug_os_unlock(drvdata); >> >> Question: Do we need a matching debug_os_lock() once we are done ? > > I have checked ARM ARMv8, but there have no detailed description for > this. I refered coresight-etmv4 code and Mike's pseudo code, ther have > no debug_os_lock() related operations. > > Mike, Mathieu, could you also help confirm this? > > [...] > >>> +static void debug_init_arch_data(void *info) >>> +{ >>> + struct debug_drvdata *drvdata = info; >>> + u32 mode, pcsr_offset; >>> + >>> + CS_UNLOCK(drvdata->base); >>> + >>> + debug_os_unlock(drvdata); >>> + >>> + /* Read device info */ >>> + drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); >>> + drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); >> >> As mentioned above, both of these registers are only need at init time to >> figure out the flags we set here. So we could remove them. >> >>> + >>> + CS_LOCK(drvdata->base); >>> + >>> + /* Parse implementation feature */ >>> + mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; >>> + pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; >> >> >>> + >>> + if (mode == EDDEVID_IMPL_NONE) { >>> + drvdata->edpcsr_present = false; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR) { >>> + drvdata->edpcsr_present = true; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { >>> + if (!IS_ENABLED(CONFIG_64BIT) && >>> + (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)) >>> + drvdata->edpcsr_present = false; >>> + else >>> + drvdata->edpcsr_present = true; >> >> Sorry, I forgot why we do this check only in this mode. Shouldn't this be >> common to all modes (of course which implies PCSR is present) ? > > No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we > simplize PCSROffset value : > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > 0001 - No offset applies. > 0010 - No offset applies, but do not use in AArch32 mode! > > So we need handle the corner case is when CPU runs AArch32 mode and > PCSRoffset = 'b0010. Other cases the pcsr should be present. I understand that reasoning. But my question is, why do we check for PCSROffset only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or any other mode where PCSR is present. Suzuki -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754623AbdC2JHT (ORCPT ); Wed, 29 Mar 2017 05:07:19 -0400 Received: from foss.arm.com ([217.140.101.70]:58878 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752459AbdC2JHO (ORCPT ); Wed, 29 Mar 2017 05:07:14 -0400 Subject: Re: [PATCH v5 6/9] coresight: add support for CPU debug module To: Leo Yan References: <1490466197-29163-1-git-send-email-leo.yan@linaro.org> <1490466197-29163-7-git-send-email-leo.yan@linaro.org> <31be033f-514e-e48a-3ba2-a5c5cd477548@arm.com> <20170329030735.GA23889@leoy-linaro> Cc: Jonathan Corbet , Rob Herring , Mark Rutland , Wei Xu , Catalin Marinas , Will Deacon , Andy Gross , David Brown , Michael Turquette , Stephen Boyd , Mathieu Poirier , Guodong Xu , John Stultz , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, linux-clk@vger.kernel.org, mike.leach@linaro.org, sudeep.holla@arm.com From: Suzuki K Poulose Message-ID: <5584203e-cb19-a5d2-45b1-3e78d3482c55@arm.com> Date: Wed, 29 Mar 2017 10:07:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170329030735.GA23889@leoy-linaro> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 29/03/17 04:07, Leo Yan wrote: > Hi Suzuki, > > On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote: >> On 25/03/17 18:23, Leo Yan wrote: > > [...] > >> Leo, >> >> Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the >> idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert >> in that area. Some minor comments below. > > Thanks a lot for quick reviewing :) > >>> Signed-off-by: Leo Yan >>> --- >>> drivers/hwtracing/coresight/Kconfig | 11 + >>> drivers/hwtracing/coresight/Makefile | 1 + >>> drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++ >>> 3 files changed, 716 insertions(+) >>> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c >>> >>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig >>> index 130cb21..18d7931 100644 >>> --- a/drivers/hwtracing/coresight/Kconfig >>> +++ b/drivers/hwtracing/coresight/Kconfig >>> @@ -89,4 +89,15 @@ config CORESIGHT_STM >>> logging useful software events or data coming from various entities >>> in the system, possibly running different OSs >>> >>> +config CORESIGHT_CPU_DEBUG >>> + tristate "CoreSight CPU Debug driver" >>> + depends on ARM || ARM64 >>> + depends on DEBUG_FS >>> + help >>> + This driver provides support for coresight debugging module. This >>> + is primarily used to dump sample-based profiling registers when >>> + system triggers panic, the driver will parse context registers so >>> + can quickly get to know program counter (PC), secure state, >>> + exception level, etc. >> >> May be we should mention/warn the user about the possible caveats of using >> this feature to help him make a better decision ? And / Or we should add a documentation >> for it. We have collected some real good information over the discussions and >> it is a good idea to capture it somewhere. > > Sure, I will add a documentation for this. > > [...] > >>> +static struct pm_qos_request debug_qos_req; >>> +static int idle_constraint = PM_QOS_DEFAULT_VALUE; >>> +module_param(idle_constraint, int, 0600); >>> +MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU " >>> + "idle states (default is -1, which means have no limiation " >>> + "to CPU idle states; 0 means disabling all idle states; user " >>> + "can choose other platform dependent values so can disable " >>> + "specific idle states for the platform)"); >> >> Correct me if I am wrong, >> >> All we want to do is disable the CPUIdle explicitly if the user knows that this >> could be a problem to use CPU debug on his platform. So, in effect, we should >> only be using idle_constraint = 0 or -1. >> >> In which case, we could make it easier for the user to tell us, either >> >> 0 - Don't do anything with CPUIdle (default) >> 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle. > > The reason for not using bool flag is: usually SoC may have many idle > states, so if user wants to partially enable some states then can set > the latency to constraint. > > But of course, we can change this to binary value as you suggested, > this means turn on of turn off all states. The only one reason to use > latency value is it is more friendly for hardware design, e.g. some > platforms can enable partial states to save power and avoid overheat > after using this driver. > > If you guys think this is a bit over design, I will follow up your > suggestion. I also have some replying in Mathieu's reviewing, please > help review as well. > >> than explaining the miscrosecond latency etc and make the appropriate calls underneath. >> something like (not necessarily the same name) : >> >> module_param(broken_with_cpuidle, bool, 0600); >> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on" >> " the platform. Non-zero value implies CPUIdle has to be" >> " explicitly disabled.",); > > [...] > >>> + /* >>> + * Unfortunately the CPU cannot be powered up, so return >>> + * back and later has no permission to access other >>> + * registers. For this case, should set 'idle_constraint' >>> + * to ensure CPU power domain is enabled! >>> + */ >>> + if (!(drvdata->edprsr & EDPRSR_PU)) { >>> + pr_err("%s: power up request for CPU%d failed\n", >>> + __func__, drvdata->cpu); >>> + goto out; >>> + } >>> + >>> +out_powered_up: >>> + debug_os_unlock(drvdata); >> >> Question: Do we need a matching debug_os_lock() once we are done ? > > I have checked ARM ARMv8, but there have no detailed description for > this. I refered coresight-etmv4 code and Mike's pseudo code, ther have > no debug_os_lock() related operations. > > Mike, Mathieu, could you also help confirm this? > > [...] > >>> +static void debug_init_arch_data(void *info) >>> +{ >>> + struct debug_drvdata *drvdata = info; >>> + u32 mode, pcsr_offset; >>> + >>> + CS_UNLOCK(drvdata->base); >>> + >>> + debug_os_unlock(drvdata); >>> + >>> + /* Read device info */ >>> + drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); >>> + drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); >> >> As mentioned above, both of these registers are only need at init time to >> figure out the flags we set here. So we could remove them. >> >>> + >>> + CS_LOCK(drvdata->base); >>> + >>> + /* Parse implementation feature */ >>> + mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; >>> + pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; >> >> >>> + >>> + if (mode == EDDEVID_IMPL_NONE) { >>> + drvdata->edpcsr_present = false; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR) { >>> + drvdata->edpcsr_present = true; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { >>> + if (!IS_ENABLED(CONFIG_64BIT) && >>> + (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)) >>> + drvdata->edpcsr_present = false; >>> + else >>> + drvdata->edpcsr_present = true; >> >> Sorry, I forgot why we do this check only in this mode. Shouldn't this be >> common to all modes (of course which implies PCSR is present) ? > > No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we > simplize PCSROffset value : > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > 0001 - No offset applies. > 0010 - No offset applies, but do not use in AArch32 mode! > > So we need handle the corner case is when CPU runs AArch32 mode and > PCSRoffset = 'b0010. Other cases the pcsr should be present. I understand that reasoning. But my question is, why do we check for PCSROffset only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or any other mode where PCSR is present. Suzuki From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suzuki.Poulose@arm.com (Suzuki K Poulose) Date: Wed, 29 Mar 2017 10:07:07 +0100 Subject: [PATCH v5 6/9] coresight: add support for CPU debug module In-Reply-To: <20170329030735.GA23889@leoy-linaro> References: <1490466197-29163-1-git-send-email-leo.yan@linaro.org> <1490466197-29163-7-git-send-email-leo.yan@linaro.org> <31be033f-514e-e48a-3ba2-a5c5cd477548@arm.com> <20170329030735.GA23889@leoy-linaro> Message-ID: <5584203e-cb19-a5d2-45b1-3e78d3482c55@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 29/03/17 04:07, Leo Yan wrote: > Hi Suzuki, > > On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote: >> On 25/03/17 18:23, Leo Yan wrote: > > [...] > >> Leo, >> >> Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the >> idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert >> in that area. Some minor comments below. > > Thanks a lot for quick reviewing :) > >>> Signed-off-by: Leo Yan >>> --- >>> drivers/hwtracing/coresight/Kconfig | 11 + >>> drivers/hwtracing/coresight/Makefile | 1 + >>> drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++ >>> 3 files changed, 716 insertions(+) >>> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c >>> >>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig >>> index 130cb21..18d7931 100644 >>> --- a/drivers/hwtracing/coresight/Kconfig >>> +++ b/drivers/hwtracing/coresight/Kconfig >>> @@ -89,4 +89,15 @@ config CORESIGHT_STM >>> logging useful software events or data coming from various entities >>> in the system, possibly running different OSs >>> >>> +config CORESIGHT_CPU_DEBUG >>> + tristate "CoreSight CPU Debug driver" >>> + depends on ARM || ARM64 >>> + depends on DEBUG_FS >>> + help >>> + This driver provides support for coresight debugging module. This >>> + is primarily used to dump sample-based profiling registers when >>> + system triggers panic, the driver will parse context registers so >>> + can quickly get to know program counter (PC), secure state, >>> + exception level, etc. >> >> May be we should mention/warn the user about the possible caveats of using >> this feature to help him make a better decision ? And / Or we should add a documentation >> for it. We have collected some real good information over the discussions and >> it is a good idea to capture it somewhere. > > Sure, I will add a documentation for this. > > [...] > >>> +static struct pm_qos_request debug_qos_req; >>> +static int idle_constraint = PM_QOS_DEFAULT_VALUE; >>> +module_param(idle_constraint, int, 0600); >>> +MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU " >>> + "idle states (default is -1, which means have no limiation " >>> + "to CPU idle states; 0 means disabling all idle states; user " >>> + "can choose other platform dependent values so can disable " >>> + "specific idle states for the platform)"); >> >> Correct me if I am wrong, >> >> All we want to do is disable the CPUIdle explicitly if the user knows that this >> could be a problem to use CPU debug on his platform. So, in effect, we should >> only be using idle_constraint = 0 or -1. >> >> In which case, we could make it easier for the user to tell us, either >> >> 0 - Don't do anything with CPUIdle (default) >> 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle. > > The reason for not using bool flag is: usually SoC may have many idle > states, so if user wants to partially enable some states then can set > the latency to constraint. > > But of course, we can change this to binary value as you suggested, > this means turn on of turn off all states. The only one reason to use > latency value is it is more friendly for hardware design, e.g. some > platforms can enable partial states to save power and avoid overheat > after using this driver. > > If you guys think this is a bit over design, I will follow up your > suggestion. I also have some replying in Mathieu's reviewing, please > help review as well. > >> than explaining the miscrosecond latency etc and make the appropriate calls underneath. >> something like (not necessarily the same name) : >> >> module_param(broken_with_cpuidle, bool, 0600); >> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on" >> " the platform. Non-zero value implies CPUIdle has to be" >> " explicitly disabled.",); > > [...] > >>> + /* >>> + * Unfortunately the CPU cannot be powered up, so return >>> + * back and later has no permission to access other >>> + * registers. For this case, should set 'idle_constraint' >>> + * to ensure CPU power domain is enabled! >>> + */ >>> + if (!(drvdata->edprsr & EDPRSR_PU)) { >>> + pr_err("%s: power up request for CPU%d failed\n", >>> + __func__, drvdata->cpu); >>> + goto out; >>> + } >>> + >>> +out_powered_up: >>> + debug_os_unlock(drvdata); >> >> Question: Do we need a matching debug_os_lock() once we are done ? > > I have checked ARM ARMv8, but there have no detailed description for > this. I refered coresight-etmv4 code and Mike's pseudo code, ther have > no debug_os_lock() related operations. > > Mike, Mathieu, could you also help confirm this? > > [...] > >>> +static void debug_init_arch_data(void *info) >>> +{ >>> + struct debug_drvdata *drvdata = info; >>> + u32 mode, pcsr_offset; >>> + >>> + CS_UNLOCK(drvdata->base); >>> + >>> + debug_os_unlock(drvdata); >>> + >>> + /* Read device info */ >>> + drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); >>> + drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); >> >> As mentioned above, both of these registers are only need at init time to >> figure out the flags we set here. So we could remove them. >> >>> + >>> + CS_LOCK(drvdata->base); >>> + >>> + /* Parse implementation feature */ >>> + mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; >>> + pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; >> >> >>> + >>> + if (mode == EDDEVID_IMPL_NONE) { >>> + drvdata->edpcsr_present = false; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR) { >>> + drvdata->edpcsr_present = true; >>> + drvdata->edcidsr_present = false; >>> + drvdata->edvidsr_present = false; >>> + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { >>> + if (!IS_ENABLED(CONFIG_64BIT) && >>> + (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)) >>> + drvdata->edpcsr_present = false; >>> + else >>> + drvdata->edpcsr_present = true; >> >> Sorry, I forgot why we do this check only in this mode. Shouldn't this be >> common to all modes (of course which implies PCSR is present) ? > > No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we > simplize PCSROffset value : > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > 0001 - No offset applies. > 0010 - No offset applies, but do not use in AArch32 mode! > > So we need handle the corner case is when CPU runs AArch32 mode and > PCSRoffset = 'b0010. Other cases the pcsr should be present. I understand that reasoning. But my question is, why do we check for PCSROffset only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or any other mode where PCSR is present. Suzuki