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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/02/2024 02:25, Tao Zhang wrote:

On 1/31/2024 6:02 PM, Suzuki K Poulose wrote:
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.

Sure.

Is it OK if I modify the code as follow and update to this patch directly?

     *val &= ~(TPDA_Pn_CR_DSBSIZE | TPDA_Pn_CR_CMBSIZE);

     if (drvdata->dsb_esize == 64)
         *val |= TPDA_Pn_CR_DSBSIZE;
     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);.
     else if (drvdata->cmb_esize == 8)
         *val &= ~TPDA_Pn_CR_CMBSIZE;

Yes, that looks good to me. Even though you would be clearing the fields for (DSB=32 and CMB=8) once again, it is good to leave it like that rather than skipping them, making people stare at it to figure out if this was correct or not.

Suzuki





Best,

Tao


Suzuki








[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux