[PATCH v3 02/11] coresight-tpda: Add DSB dataset support
Suzuki K Poulose
suzuki.poulose at arm.com
Tue Mar 28 05:33:20 PDT 2023
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 at 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
>>>>
>>>>
>>
More information about the linux-arm-kernel
mailing list