| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <571E37BA.5080703@arm.com>
Date: Mon, 25 Apr 2016 16:28:58 +0100
From: Suzuki K Poulose <Suzuki.Poulose@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width
On 25/04/16 16:25, Mathieu Poirier wrote:
> On 25 April 2016 at 09:09, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
>> On 25/04/16 15:55, Mathieu Poirier wrote:
>>>
>>> On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@....com>
>>> 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 <mathieu.poirier@...aro.org>
>>>>> ---
>>>>> 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
Powered by blists - more mailing lists