Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support
From: Tao Zhang
Date: Thu Mar 30 2023 - 10:08:22 EST
Hi Suzuki,
On 3/28/2023 8:33 PM, Suzuki K Poulose wrote:
On 28/03/2023 12:31, Tao Zhang wrote:
Hi Suzuki,
On 3/27/2023 5:43 PM, Suzuki K Poulose wrote:
On 27/03/2023 04:31, Tao Zhang wrote:
On 3/26/2023 3:31 AM, Suzuki K Poulose wrote:
On 24/03/2023 14:58, Tao Zhang wrote:
Hi Suzuki,
在 3/23/2023 7:51 PM, Suzuki K Poulose 写道:
On 23/03/2023 06:03, 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-tpda.c | 58
++++++++++++++++++++++++++++
drivers/hwtracing/coresight/coresight-tpda.h | 4 ++
2 files changed, 62 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
b/drivers/hwtracing/coresight/coresight-tpda.c
index f712e11..8dcfc4a 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -21,6 +21,47 @@
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.
+ */
I am still not clear why we need to do this recursively ?
Some TPDMs are not directly output connected to the TPDAs. So here I
use a recursive method to check from the TPDA input port until I
find
the connected TPDM.
Do you have a better suggestion besides a recursive method?
+static int tpda_set_element_size(struct tpda_drvdata *drvdata,
+ struct coresight_device *csdev, int inport,
bool parent)
Please could we renamse csdev => tpda_dev
Since this is a recursively called function, this Coresight
device is not
necessarily TPDA, it can be other Coresight device.
+{
+ static int nr_inport;
+ int i;
+ struct coresight_device *in_csdev;
similarly tpdm_dev ?
Same as above, this variable may not necessarily be a TPDM.
Could we not add a check here to see if the dsb_esize[inport] is
already
set and then bail out, reading this over and over ?
I will update this in the next patch series.
+
+ if (inport > (TPDA_MAX_INPORTS - 1))
+ return -EINVAL;
+
+ if (parent)
+ nr_inport = inport;
+
+ for (i = 0; i < csdev->pdata->nr_inconns; i++) {
+ in_csdev = csdev->pdata->in_conns[i].remote_dev;
Please note, the names of the structure field might change in the
next version of James' series
Got it. I will keep an eye out for the James' patch series.
+ if (!in_csdev)
+ break;
+
+ if (parent)
+ if (csdev->pdata->in_conns[i].port != inport)
+ continue;
+
+ if (in_csdev && strstr(dev_name(&in_csdev->dev),
"tpdm")) {
Isn't there a better way to distinguish a device to be TPDM ?
May be we
could even add a source_sub_type - SOURCE_TPDM instead of using
SOURCE_OTHERS ? Do you expect other sources to be connected to
TPDA?
e.g., STMs ?
I can add "SOURCE_TPDM" as a source_sub_type, but SOURCE_OTHERS
needs
to be kept since the other Coresight component we will upstream
later may
need it.
+ of_property_read_u32(in_csdev->dev.parent->of_node,
+ "qcom,dsb-element-size",
&drvdata->dsb_esize[nr_inport]);
+ break;
+ }
+ tpda_set_element_size(drvdata, in_csdev, 0, false);
What is the point of this ? Is this for covering the a TPDA
connected to
another TPDA ?
e.g., { TPDM0, TPDM1 } -> TPDA0 -> TPDA1 ?
A TPDM may not connect to the TPDA directly, for example,
TPDM0 ->FUNNEL0->FUNNEL1->TPDA0
And many TPDMs can connect to one TPDA, one input port on TPDA
only has
one TPDM connected. Therefore, we use a recursive method to find
the TPDM
corresponding to the input port of TPDA.
How do you find out decide what to choose, if there are multiple
TPDMs
connected to FUNNEL0 or even FUNNEL1 ?
e.g
TPDM0->FUNNEL0->FUNNEL1->TPDA0
/
TPDM1
We can find out the corresponding TPDM by the input port number of
TPDA.
Each input port is connected to a TPDM. So we have an input port
number in
the input parameter of the recursive lookup function
"tpda_set_element_size".
I don't understand, how you would figure out, in the above situation.
i.e., FUNNEL1 is connected to TPDA0, but there are two TPDMs that could
be pumping the trace. They both arrive via FUNNEL1. So, how does that
solve your problem ?
In our HW design, the input ports of TPDA and TPDM are one-one-one
corresponding. Only one
TPDM can be found connected from one TPDA's input port. The path to a
TPDA input port doesn't
connect more than one TPDM. It's by HW design.
Your current designs may be like that. But as far as the driver is
concerned, I would like to add in extra measures to ensure that it
encounters a variation from the above on a future platform. So, please
could you add a check to detect this case and add a WARNING ?
Got it, I will update it according to your advice in the next patch series.
Tao
Suzuki
Tao
Suzuki
Suzuki