Messages in this thread | | | Date | Mon, 27 Mar 2023 11:31:54 +0800 | Subject | Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support | From | Tao Zhang <> |
| |
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".
> Suzuki > >
| |