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

  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(+)
  

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 ?

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

> +{
> +	static int nr_inport;
> +	int i;
> +	struct coresight_device *in_csdev;

similarly tpdm_dev ?

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 ?

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

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

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

And you want to figure out the DSB size of TPDM0 when you want to enable
TPDA1 ? How do you choose between that size of TPDM0 vs TPDM1 ?

Please add a proper documentation for this function ? If TPDA0 is in the
the path, it should have been enabled before you reach TPDA1. Thus,
the dsb_esize array must have been initialised for TPDA0 and thus, you
could simply read it from the dsb_esize[] of TPDA0.
You could always look at the device_type and sub_type to detect a
TPDA{0} connected into TPDA{1}



> +	}
> +
> +	return 0;
> +}
> +
>   /* Settings pre enabling port control register */
>   static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   {
> @@ -37,6 +78,18 @@ static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>   	u32 val;
>   
>   	val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
> +	/*
> +	 * Configure aggregator port n DSB data set element size
> +	 * Set the bit to 0 if the size is 32
> +	 * Set the bit to 1 if the size is 64
> +	 */
> +	if (drvdata->dsb_esize[port] == 32)
> +		val &= ~TPDA_Pn_CR_DSBSIZE;
> +	else if (drvdata->dsb_esize[port] == 64)
> +		val |= TPDA_Pn_CR_DSBSIZE;
> +	else
> +		dev_err(drvdata->dev,
> +			"DSB data size input from port[%d] is invalid\n", port);

WARN_ON_ONCE() and abort the enable opration ? Or say, "fallback to 
32bit or 64bit" if one of them is a safer option ? Please don't
leave it unknown.

>   	/* Enable the port */
>   	val |= TPDA_Pn_CR_ENA;
>   	writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> @@ -57,6 +110,11 @@ static void __tpda_enable(struct tpda_drvdata *drvdata, int port)
>   static int tpda_enable(struct coresight_device *csdev, int inport, int outport)
>   {
>   	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	int ret;
> +
> +	ret = tpda_set_element_size(drvdata, csdev, inport, true);
> +	if (ret)
> +		return ret;
>   
>   	spin_lock(&drvdata->spinlock);
>   	if (atomic_read(&csdev->refcnt[inport]) == 0)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index 0399678..9ec5870 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -10,6 +10,8 @@
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>   /* Aggregator port enable bit */
>   #define TPDA_Pn_CR_ENA		BIT(0)
> +/* Aggregator port DSB data set element size bit */
> +#define TPDA_Pn_CR_DSBSIZE		BIT(8)
>   
>   #define TPDA_MAX_INPORTS	32
>   
> @@ -23,6 +25,7 @@
>    * @csdev:      component vitals needed by the framework.
>    * @spinlock:   lock for the drvdata value.
>    * @enable:     enable status of the component.
> + * @dsb_esize:   DSB element size

Please state, must be 32 or 64.

>    */
>   struct tpda_drvdata {
>   	void __iomem		*base;
> @@ -30,6 +33,7 @@ struct tpda_drvdata {
>   	struct coresight_device	*csdev;
>   	spinlock_t		spinlock;
>   	u8			atid;
> +	u32			dsb_esize[TPDA_MAX_INPORTS];

Couldn't this be u8 ?


Suzuki

>   };
>   
>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
  

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
Suzuki
  

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

> Suzuki
>
>
  

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

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.


Tao

>
> Suzuki
>
>
>>
>>> Suzuki
>>>
>>>
>
  

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

Hi Suzuki,

On 3/28/2023 8:33 PM, Suzuki K Poulose wrote:
> 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 ?

Got it, I will update it according to your advice in the next patch series.


Tao

>
> Suzuki
>
>
>>
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>>
>>>>> Suzuki
>>>>>
>>>>>
>>>
>