From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932843AbcDYP3F (ORCPT ); Mon, 25 Apr 2016 11:29:05 -0400 Received: from foss.arm.com ([217.140.101.70]:47417 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932511AbcDYP3B (ORCPT ); Mon, 25 Apr 2016 11:29:01 -0400 Subject: Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width To: Mathieu Poirier References: <1461345255-11758-1-git-send-email-mathieu.poirier@linaro.org> <1461345255-11758-15-git-send-email-mathieu.poirier@linaro.org> <571E2C89.7010003@arm.com> <571E3343.9090705@arm.com> Cc: "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" From: Suzuki K Poulose Message-ID: <571E37BA.5080703@arm.com> Date: Mon, 25 Apr 2016 16:28:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: 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 25/04/16 16:25, Mathieu Poirier wrote: > On 25 April 2016 at 09:09, Suzuki K Poulose wrote: >> On 25/04/16 15:55, Mathieu Poirier wrote: >>> >>> On 25 April 2016 at 08:41, Suzuki K Poulose >>> wrote: >>>> >>>> On 22/04/16 18:14, Mathieu Poirier wrote: >>>>> >>>>> >>>>> Accessing the HW configuration register each time the memory >>>>> width is needed simply doesn't make sense. It is much more >>>>> efficient to read the value once and keep a reference for >>>>> later use. >>>>> >>>>> Signed-off-by: Mathieu Poirier >>>>> --- >>>>> drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +--------- >>>>> drivers/hwtracing/coresight/coresight-tmc.c | 34 >>>>> +++++++++++++++++++++++++ >>>>> drivers/hwtracing/coresight/coresight-tmc.h | 10 +++++--- >>>>> 3 files changed, 41 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c >>>>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c >>>>> index cc88c15ba45c..981c5ca75e36 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c >>>>> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata) >>>>> >>>>> static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata) >>>>> { >>>>> - enum tmc_mem_intf_width memwidth; >>>>> - u8 memwords; >>>>> char *bufp; >>>>> u32 read_data; >>>>> int i; >>>>> >>>>> - memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID), >>>>> 8, 10); >>>>> - if (memwidth == TMC_MEM_INTF_WIDTH_32BITS) >>>>> - memwords = 1; >>>>> - else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS) >>>>> - memwords = 2; >>>>> - else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS) >>>>> - memwords = 4; >>>>> - else >>>>> - memwords = 8; >>>>> - >>>>> bufp = drvdata->buf; >>>>> while (1) { >>>>> - for (i = 0; i < memwords; i++) { >>>>> + for (i = 0; i < drvdata->memwidth; i++) { >>>>> read_data = readl_relaxed(drvdata->base + >>>>> TMC_RRD); >>>>> if (read_data == 0xFFFFFFFF) >>>>> return; >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c >>>>> b/drivers/hwtracing/coresight/coresight-tmc.c >>>>> index 55806352b1f1..cb030a09659d 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c >>>>> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = { >>>>> .llseek = no_llseek, >>>>> }; >>>>> >>>>> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) >>>>> +{ >>>>> + enum tmc_mem_intf_width memwidth; >>>>> + >>>>> + /* >>>>> + * Excerpt from the TRM: >>>>> + * >>>>> + * DEVID::MEMWIDTH[10:8] >>>>> + * 0x2 Memory interface databus is 32 bits wide. >>>>> + * 0x3 Memory interface databus is 64 bits wide. >>>>> + * 0x4 Memory interface databus is 128 bits wide. >>>>> + * 0x5 Memory interface databus is 256 bits wide. >>>>> + */ >>>>> + switch (BMVAL(devid, 8, 10)) { >>>>> + case 0x2: >>>>> + memwidth = TMC_MEM_INTF_WIDTH_32BITS; >>>>> + break; >>>>> + case 0x3: >>>>> + memwidth = TMC_MEM_INTF_WIDTH_64BITS; >>>>> + break; >>>>> + case 0x4: >>>>> + memwidth = TMC_MEM_INTF_WIDTH_128BITS; >>>>> + break; >>>>> + case 0x5: >>>>> + memwidth = TMC_MEM_INTF_WIDTH_256BITS; >>>>> + break; >>>>> + default: >>>>> + memwidth = 0; >>>>> + } >>>>> + >>>>> + return memwidth; >>>>> +} >>>>> + >>>>> #define coresight_tmc_simple_func(name, offset) >>>>> \ >>>>> coresight_simple_func(struct tmc_drvdata, name, offset) >>>>> >>>>> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const >>>>> struct amba_id *id) >>>>> >>>>> devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID); >>>>> drvdata->config_type = BMVAL(devid, 6, 7); >>>>> + drvdata->memwidth = tmc_get_memwidth(devid); >>>>> >>>>> if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { >>>>> if (np) >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h >>>>> b/drivers/hwtracing/coresight/coresight-tmc.h >>>>> index 9b4c215d2b6b..12a097f3d06c 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h >>>>> @@ -81,10 +81,10 @@ enum tmc_mode { >>>>> }; >>>>> >>>>> enum tmc_mem_intf_width { >>>>> - TMC_MEM_INTF_WIDTH_32BITS = 0x2, >>>>> - TMC_MEM_INTF_WIDTH_64BITS = 0x3, >>>>> - TMC_MEM_INTF_WIDTH_128BITS = 0x4, >>>>> - TMC_MEM_INTF_WIDTH_256BITS = 0x5, >>>>> + TMC_MEM_INTF_WIDTH_32BITS = 1, >>>>> + TMC_MEM_INTF_WIDTH_64BITS = 2, >>>>> + TMC_MEM_INTF_WIDTH_128BITS = 4, >>>>> + TMC_MEM_INTF_WIDTH_256BITS = 8, >>>>> }; >>>> >>>> >>>> >>>> I think this would cause confusion for the reader. It would be good to >>>> leave the definitions above as before and tmc_get_memwidth() doing: >>>> >>>> i.e, >>>> case TMC_MEM_INTF_WIDTH_32BITS: >>>> memwidth = 1; >>>> break; >>>> >>>> But we still store the memwidth in bytes. >>> >>> >>> If we proceed this way we have to do a case statement on hard coded >>> values (or introduce a new enumeration) in tmc_update_etf_buffer(). In >> >> >> But then we are doing that already with this patch in tmc_get_memwidth >> already. > > Right. With my approach we use hard coded values once and named > symbols every time we need access to memwidth. With your approach we > use name symbols once and hard coded values (along with a lengthy > comment) every time memwidth is accessed. That lengthy comment already exists as part of your patch. All I was saying is you could reduce the switch..case for width to an if () for hard coded value check, which is only done for mask generation. Suzuki