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

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

On 25/05/2023 08:16, Tao Zhang wrote:
> 
> On 5/23/2023 6:07 PM, Suzuki K Poulose wrote:
>> On 27/04/2023 10:00, 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-core.c |  1 +
>>>   drivers/hwtracing/coresight/coresight-tpda.c | 92 
>>> +++++++++++++++++++++++++---
>>>   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c |  2 +-
>>>   include/linux/coresight.h                    |  1 +
>>>   5 files changed, 90 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 2af416b..f1eacbb 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct 
>>> coresight_device *csdev,
>>>         if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
>>> +        subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
>>>           subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
>>>           dev_err(&csdev->dev, "wrong device subtype in %s\n", 
>>> function);
>>>           return -EINVAL;
>>
>> Please see the comment at the bottom.
>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>> index 8d2b9d2..af9c72f 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -21,6 +21,56 @@
>>>     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.
>>> + */
>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata,
>>> +               struct coresight_device *csdev, int inport, bool parent)
>>> +{
>>> +    static int nr_inport;
>>> +    int i;
>>> +    static bool tpdm_found;
>>> +    struct coresight_device *in_csdev;
>>> +
>>> +    if (inport > (TPDA_MAX_INPORTS - 1))
>>> +        return -EINVAL;
>>> +
>>> +    if (parent) {
>>> +        nr_inport = inport;
>>> +        tpdm_found = false;
>>> +    }
>>> +
>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>> +        in_csdev = csdev->pdata->in_conns[i]->src_dev;
>>> +        if (!in_csdev)
>>> +            break;
>>> +
>>> +        if (parent)
>>> +            if (csdev->pdata->in_conns[i]->dest_port != inport)
>>> +                continue;
>>> +
>>> +        if (in_csdev->subtype.source_subtype
>>
>> We must match the in_csdev->type to be SOURCE && the subtype.
> Sure, I will update it in the next patch series.
>>
>>> +                   == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) {
>>> + of_property_read_u8(in_csdev->dev.parent->of_node,
>>> +                    "qcom,dsb-element-size", 
>>> &drvdata->dsb_esize[nr_inport]);
>>> +            if (!tpdm_found)
>>> +                tpdm_found = true;
>>> +            else
>>> +                dev_warn(drvdata->dev,
>>> +                    "More than one TPDM is mapped to the TPDA input 
>>> port %d.\n",
>>> +                    nr_inport);
>>
>> When we know, we have found a source device, we don't need to recurse
>> down and could simply 'continue' to the next one in the list and skip
>> the call below.
> 
> Actually, one input port on TPDA only can connect to one TPDM. In the 
> current design, it will
> 
> find out all the TPDMs on one input port and warn the users all the 
> TPDMs it found. If we
> 
> replace 'recurse down' as 'continue' here, it may not find some TPDMs 
> that might be connected
> 
> incorrectly.


What do you mean ? When you enter the if () above, the in_csdev is a
source and it is TPDM. There must be no input connections TPDM, i.e.
in_csdev, so no need to go further up the connection chain looking at
the in_csdev. The loop will continue to analyse this device (where we
found one TPDM already) and detect any further duplicate TPDMs
connected.

Suzuki