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

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

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
>>>>
>>>>
>>