diff mbox

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

Message ID 1461345255-11758-15-git-send-email-mathieu.poirier@linaro.org
State Superseded
Headers show

Commit Message

Mathieu Poirier April 22, 2016, 5:14 p.m. UTC 
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(-)

-- 
2.5.0

Comments

Mathieu Poirier April 25, 2016, 2:55 p.m. UTC | #1 
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
my opinion the current solution scales better.

Mathieu


>

> Suzuki
Mathieu Poirier April 25, 2016, 3:25 p.m. UTC | #2 
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.

> My point is, we expect named symbols for values defined in the standards, so

> that

> you don't make a mistake when dealing with them. It is really difficult to

> track

> down a bug if we do make a mistake.

>

>> my opinion the current solution scales better.

>

>

> I would prefer doing a hardcoded check in tmc_update_etf_buffer() like :

>

> +               /*

> +                * The value written to RRP must be byte-address aligned to

> +                * the width of the trace memory databus _and_ to a frame

> +                * boundary (16 byte), whichever is the biggest. For

> example,

> +                * for 32-bit, 64-bit and 128-bit wide trace memory, the

> four

> +                * LSBs must be 0s. For 256-bit wide trace memory, the five

> +                * LSBs must be 0s.

> +                */

> +               if (drvdata->memwidth == 8)

> +                       mask = GENMASK(31, 6);

> +               else

> +                       mask = GENMASK(31, 5);

>

>

> Suzuki
diff mbox

Patch

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,
 };
 
 /**
@@ -101,6 +101,7 @@  enum tmc_mem_intf_width {
  * @size:	@buf size.
  * @mode:	how this TMC is being used.
  * @config_type: TMC variant, must be of type @tmc_config_type.
+ * @memwidth:	width of the memory interface databus, in bytes.
  * @trigger_cntr: amount of words to store after a trigger.
  */
 struct tmc_drvdata {
@@ -117,6 +118,7 @@  struct tmc_drvdata {
 	u32			size;
 	local_t			mode;
 	enum tmc_config_type	config_type;
+	enum tmc_mem_intf_width	memwidth;
 	u32			trigger_cntr;
 };