Re: [PATCH v5 05/10] coresight-tpda: Add support to configure CMB element

From: Suzuki K Poulose
Date: Wed Jan 31 2024 - 05:04:57 EST


On 31/01/2024 01:39, Tao Zhang wrote:

On 1/30/2024 8:35 PM, Suzuki K Poulose wrote:
On 30/01/2024 09:02, Tao Zhang wrote:
Read the CMB element size from the device tree. Set the register
bit that controls the CMB element size of the corresponding port.

Reviewed-by: James Clark <james.clark@xxxxxxx>
Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
Signed-off-by: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
---
  drivers/hwtracing/coresight/coresight-tpda.c | 123 +++++++++++--------
  drivers/hwtracing/coresight/coresight-tpda.h |   6 +
  2 files changed, 79 insertions(+), 50 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 4ac954f4bc13..fcddff3ded81 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -18,6 +18,7 @@
  #include "coresight-priv.h"
  #include "coresight-tpda.h"
  #include "coresight-trace-id.h"
+#include "coresight-tpdm.h"
    DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
  @@ -28,24 +29,57 @@ static bool coresight_device_is_tpdm(struct coresight_device *csdev)
              CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
  }
  +static void tpdm_clear_element_size(struct coresight_device *csdev)
+{
+    struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+    drvdata->dsb_esize = 0;
+    drvdata->cmb_esize = 0;
+}
+
+static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 *val)
+{
+



+    if (drvdata->dsb_esize == 64)
+        *val |= TPDA_Pn_CR_DSBSIZE;

We don't seem to be clearing the fields we modify, before updating them. This may be OK in real world where the device connected to TPDA port
may not change. But it is always safer to clear the bits and set it.

e.g.:
    *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);



+    else if (drvdata->dsb_esize == 32)
+        *val &= ~TPDA_Pn_CR_DSBSIZE;
+
+    if (drvdata->cmb_esize == 64)
+        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
+    else if (drvdata->cmb_esize == 32)
+        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);

Similarly here ^^^. I am happy to fix it up if you are OK with it (unless there are other changes that need a respin)

Thank you. I would be very grateful if you could help for this.

Given, you need to respin, please incorporate this change too.

Suzuki