Re: [PATCH v3 02/11] coresight-tpda: Add DSB dataset support

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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 ?

Suzuki


> 
>> Suzuki
>>
>>