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