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

[Mathieu Poirier]

On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@xxxxxxx> 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@xxxxxxxxxx>
>> ---
>>   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
my opinion the current solution scales better.

Mathieu


>
> Suzuki