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
