[PATCH V3 14/18] coresight: tmc: keep track of memory width

[Suzuki.Poulose@arm.com (Suzuki K Poulose)]

On 25/04/16 16:25, Mathieu Poirier wrote:
> On 25 April 2016 at 09:09, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 25/04/16 15:55, Mathieu Poirier wrote:
>>>
>>> On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@arm.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@linaro.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