Re: [PATCH v4 02/11] coresight-tpda: Add DSB dataset support

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

 




On 5/23/2023 6:07 PM, Suzuki K Poulose wrote:
On 27/04/2023 10:00, Tao Zhang wrote:
Read the DSB element size from the device tree. Set the register
bit that controls the DSB element size of the corresponding port.

Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
---
  drivers/hwtracing/coresight/coresight-core.c |  1 +
  drivers/hwtracing/coresight/coresight-tpda.c | 92 +++++++++++++++++++++++++---
  drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
  drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
  include/linux/coresight.h                    |  1 +
  5 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 2af416b..f1eacbb 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
        if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
          subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
+        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
          subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
          dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
          return -EINVAL;

Please see the comment at the bottom.

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 8d2b9d2..af9c72f 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -21,6 +21,56 @@
    DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
  +/* Search and read element data size from the TPDM node in
+ * the devicetree. Each input port of TPDA is connected to
+ * a TPDM. Different TPDM supports different types of dataset,
+ * and some may support more than one type of dataset.
+ * Parameter "inport" is used to pass in the input port number
+ * of TPDA, and it is set to 0 in the recursize call.
+ * Parameter "parent" is used to pass in the original call.
+ */
+static int tpda_set_element_size(struct tpda_drvdata *drvdata,
+               struct coresight_device *csdev, int inport, bool parent)
+{
+    static int nr_inport;
+    int i;
+    static bool tpdm_found;
+    struct coresight_device *in_csdev;
+
+    if (inport > (TPDA_MAX_INPORTS - 1))
+        return -EINVAL;
+
+    if (parent) {
+        nr_inport = inport;
+        tpdm_found = false;
+    }
+
+    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
+        in_csdev = csdev->pdata->in_conns[i]->src_dev;
+        if (!in_csdev)
+            break;
+
+        if (parent)
+            if (csdev->pdata->in_conns[i]->dest_port != inport)
+                continue;
+
+        if (in_csdev->subtype.source_subtype

We must match the in_csdev->type to be SOURCE && the subtype.
Sure, I will update it in the next patch series.

+                   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
+ of_property_read_u8(in_csdev->dev.parent->of_node,
+                    "qcom,dsb-element-size", &drvdata->dsb_esize[nr_inport]);
+            if (!tpdm_found)
+                tpdm_found = true;
+            else
+                dev_warn(drvdata->dev,
+                    "More than one TPDM is mapped to the TPDA input port %d.\n",
+                    nr_inport);

When we know, we have found a source device, we don't need to recurse
down and could simply 'continue' to the next one in the list and skip
the call below.

Actually, one input port on TPDA only can connect to one TPDM. In the current design, it will

find out all the TPDMs on one input port and warn the users all the TPDMs it found. If we

replace 'recurse down' as 'continue' here, it may not find some TPDMs that might be connected

incorrectly.


+        }
+        tpda_set_element_size(drvdata, in_csdev, 0, false);
+    }
+
+    return 0;
+}
+
  /* Settings pre enabling port control register */
  static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
  {
@@ -32,26 +82,43 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
      writel_relaxed(val, drvdata->base + TPDA_CR);
  }
  -static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
+static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
  {
      u32 val;
        val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
+    /*
+     * Configure aggregator port n DSB data set element size
+     * Set the bit to 0 if the size is 32
+     * Set the bit to 1 if the size is 64
+     */
+    if (drvdata->dsb_esize[port] == 32)
+        val &= ~TPDA_Pn_CR_DSBSIZE;
+    else if (drvdata->dsb_esize[port] == 64)
+        val |= TPDA_Pn_CR_DSBSIZE;
+    else
+        return -EINVAL;
+
      /* Enable the port */
      val |= TPDA_Pn_CR_ENA;
      writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
+
+    return 0;
  }
  -static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
+static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
  {
+    int ret;
+
      CS_UNLOCK(drvdata->base);
        if (!drvdata->csdev->enable)
          tpda_enable_pre_port(drvdata);
  -    tpda_enable_port(drvdata, port);
-
+    ret = tpda_enable_port(drvdata, port);
      CS_LOCK(drvdata->base);
+
+    return ret;
  }
    static int tpda_enable(struct coresight_device *csdev,
@@ -59,16 +126,23 @@ static int tpda_enable(struct coresight_device *csdev,
                 struct coresight_connection *out)
  {
      struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+    int ret;
+
+    ret = tpda_set_element_size(drvdata, csdev, in->dest_port, true);
+    if (ret)
+        return ret;
        spin_lock(&drvdata->spinlock);
-    if (atomic_read(&in->dest_refcnt) == 0)
+    if (atomic_read(&in->dest_refcnt) == 0) {
          __tpda_enable(drvdata, in->dest_port);

ret = ... ?
Sure, I will update it in the next patch series.

+        if (!ret) {



+ atomic_inc(&in->dest_refcnt);
+            dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
+        }
+    }

  - atomic_inc(&in->dest_refcnt);

This seems wrong, as we may fail to hold additional refcounts for the
additional sessions ?

In the current design, if the TPDA is enabled successfully, it will run "atomic_inc(&in->dest_refcnt);"

Otherwise, it will not run "atomic_inc(&in->dest_refcnt);" to avoid additional refcounts. Is it right?


spin_unlock(&drvdata->spinlock);
-
-    dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
-    return 0;
+    return ret;
  }
    static void __tpda_disable(struct tpda_drvdata *drvdata, int port)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index 0399678..7332e9c 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -10,6 +10,8 @@
  #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
  /* Aggregator port enable bit */
  #define TPDA_Pn_CR_ENA        BIT(0)
+/* Aggregator port DSB data set element size bit */
+#define TPDA_Pn_CR_DSBSIZE        BIT(8)
    #define TPDA_MAX_INPORTS    32
  @@ -23,6 +25,7 @@
   * @csdev:      component vitals needed by the framework.
   * @spinlock:   lock for the drvdata value.
   * @enable:     enable status of the component.
+ * @dsb_esize:  DSB element size, it must be 32 or 64.

minor nit:

DSB element size for each inport, it must be 32 or 64
Sure, I will update it in the next patch series.

   */
  struct tpda_drvdata {
      void __iomem        *base;
@@ -30,6 +33,7 @@ struct tpda_drvdata {
      struct coresight_device    *csdev;
      spinlock_t        spinlock;
      u8            atid;
+    u8            dsb_esize[TPDA_MAX_INPORTS];
  };
    #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index f4854af..ba1867f 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -205,7 +205,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
      if (!desc.name)
          return -ENOMEM;
      desc.type = CORESIGHT_DEV_TYPE_SOURCE;
-    desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
+    desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM;
      desc.ops = &tpdm_cs_ops;
      desc.pdata = adev->dev.platform_data;
      desc.dev = &adev->dev;

Please could you split this change, i.e., introduction of
SUBTYPE_SOURCE_TPDM and using this in TPDM driver,
 in a separate patch before this change.

Sure, I will update it in the next patch series.


Best,

Tao


diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 225a5fa..6563896 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -60,6 +60,7 @@ enum coresight_dev_subtype_source {
      CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
      CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
      CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
+    CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM,
      CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS,
  };




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux