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@quicinc.com >> --- >> 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