Messages in this thread | | | Date | Tue, 28 Mar 2023 13:33:20 +0100 | Subject | Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support | From | Suzuki K Poulose <> |
| |
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 ?
Suzuki
> > > Tao > >> >> Suzuki >> >> >>> >>>> Suzuki >>>> >>>> >>
| |