Re: [PATCH v2 2/8] coresight-tpda: Add support to configure CMB element

[Tao Zhang <quic_taozha@quicinc.com>]

On 11/1/2023 7:36 PM, James Clark wrote:
>
>
> On 01/11/2023 08:53, Tao Zhang wrote:
>>
>> On 10/30/2023 7:11 PM, James Clark wrote:
>>>
>>> On 25/10/2023 03:53, Tao Zhang wrote:
>>>> Read the CMB element size from the device tree. Set the register
>>>> bit that controls the CMB element size of the corresponding port.
>>>>
>>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
>>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-tpda.c | 108
>>>> ++++++++++++++++-----------
>>>>   drivers/hwtracing/coresight/coresight-tpda.h |   6 ++
>>>>   2 files changed, 69 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> index 5f82737..3101d2a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct
>>>> coresight_device *csdev)
>>>>               CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM);
>>>>   }
>>>>   +static void tpdm_clear_element_size(struct coresight_device *csdev)
>>>> +{
>>>> +    struct tpda_drvdata *drvdata = 
>>>> dev_get_drvdata(csdev->dev.parent);
>>>> +
>>>> +    if (drvdata->dsb_esize)
>>>> +        drvdata->dsb_esize = 0;
>>>> +    if (drvdata->cmb_esize)
>>>> +        drvdata->cmb_esize = 0;
>
> The ifs don't do anything here, it's just the same as always setting 
> to 0.
I will update in the next patch series.
>
>>>> +}
>>>> +
>>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32
>>>> *val)
>>>> +{
>>>> +
>>>> +    if (drvdata->dsb_esize == 64)
>>>> +        *val |= TPDA_Pn_CR_DSBSIZE;
>>>> +    else if (drvdata->dsb_esize == 32)
>>>> +        *val &= ~TPDA_Pn_CR_DSBSIZE;
>>>> +
>>>> +    if (drvdata->cmb_esize == 64)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2);
>>>> +    else if (drvdata->cmb_esize == 32)
>>>> +        *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1);
>>>> +    else if (drvdata->cmb_esize == 8)
>>>> +        *val &= ~TPDA_Pn_CR_CMBSIZE;
>>>> +}
>>>> +
>>>>   /*
>>>>    * Read the DSB element size from the TPDM device
>>>>    * Returns
>>>>    *    The dsb element size read from the devicetree if available.
>>> Hi Tao,
>>>
>>> I think the function description and the return value description above
>>> need updating now.
>> I will update this in the next patch series.
>>>
>>>>    *    0 - Otherwise, with a warning once.
>>>>    */
>>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev)
>>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata,
>>>> +                  struct coresight_device *csdev)
>>>>   {
>>>> -    int rc = 0;
>>>> -    u8 size = 0;
>>>> -
>>>> -    rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> -            "qcom,dsb-element-size", &size);
>>>> +    int rc = -EEXIST;
>>>> +
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,dsb-element-size", &drvdata->dsb_esize))
>>>> +        rc &= 0;
>>> Is &= 0 significant? Why not = 0?
>> I will update this in the next patch series.
>>>
>>>> +    if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent),
>>>> +            "qcom,cmb-element-size", &drvdata->cmb_esize))
>>>> +        rc &= 0;
>>>>       if (rc)
>>>>           dev_warn_once(&csdev->dev,
>>>> -            "Failed to read TPDM DSB Element size: %d\n", rc);
>>>> +            "Failed to read TPDM Element size: %d\n", rc);
>>>>   -    return size;
>>>> +    return rc;
>>>>   }
>>>>     /*
>>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct
>>>> coresight_device *csdev)
>>>>    * Parameter "inport" is used to pass in the input port number
>>>>    * of TPDA, and it is set to -1 in the recursize call.
>>>>    */
>>>> -static int tpda_get_element_size(struct coresight_device *csdev,
>>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata,
>>>> +                 struct coresight_device *csdev,
>>>>                    int inport)
>>>>   {
>>>> -    int dsb_size = -ENOENT;
>>>> -    int i, size;
>>>> +    int rc = -ENOENT;
>>>> +    int i;
>>>>       struct coresight_device *in;
>>>>   +    if (inport > 0)
>>>> +        tpdm_clear_element_size(csdev);
>>>> +
>>>>       for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>>           in = csdev->pdata->in_conns[i]->src_dev;
>>>>           if (!in)
>>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct
>>>> coresight_device *csdev,
>>>>               continue;
>>>>             if (coresight_device_is_tpdm(in)) {
>>>> -            size = tpdm_read_dsb_element_size(in);
>>>> +            if (rc)
>>>> +                rc = tpdm_read_element_size(drvdata, in);
>>>> +            else
>>>> +                return -EINVAL;
>>> This is quite hard to follow, is checking rc here before calling
>>> tpdm_read_element_size() some kind of way of only calling it once?
>>
>> Yes, there can only be one TPDM on one input port of TPDA. If
>> "tpdm_read_element_size" is called
>>
>> twice, it means that two TPDMs are found on one input port of TPDA.
>>
>>> rc isn't actually a return value at this point, it's just default
>>> initialised to -ENOENT.
>>
>> In this loop, "tpdm_read_element_size" will be called for the first time
>> TPDM found. If rc equals to
>>
>> 0, it means that at lease one TPDM has been found on the input port.
>> Does it make sense?
>>
>>> Then it's not clear why the else condition returns an error?
>> The same as the above.
>>>
>>>>           } else {
>>>>               /* Recurse down the path */
>>>> -            size = tpda_get_element_size(in, -1);
>>>> -        }
>>>> -
>>>> -        if (size < 0)
>>>> -            return size;
>>>> -
>>>> -        if (dsb_size < 0) {
>>>> -            /* Found a size, save it. */
>>>> -            dsb_size = size;
>>>> -        } else {
>>>> -            /* Found duplicate TPDMs */
>>>> -            return -EEXIST;
>>>> +            rc = tpda_get_element_size(drvdata, in, -1);
>>> And then why it's assigned here but not checked for an error in this
>>> case?
>>
>> |Remote ETM|                           |TPDM|
>>
>>             |    branch0                           | branch1
>>
>>                         --------------------------
>>
>>                                     funnel1
>>
>>                         ---------------------------
>>
>>                                           | input port 1
>>
>> -----------------------------
>>
>>                                                        TPDA1
>>
>> -----------------------------
>>
>> The  branches on some TPDA's input ports may not have TPDM. For example,
>> branch 0 doesn't has a
>>
>> TPDM connected, tpda_get_element_size will not return 0 here. This is a
>> normal situation. We need to
>>
>> continue to find TPDM on branch1 through recursion.
>>
>>>
>>> Maybe some comments explaining the flow would help. Or to me it seems
>>> like a second variable to track the thing that's actually intended 
>>> could
>>> be used as well. Like "bool found_element" or something, rather than
>>> re-using rc to also track another thing.
>>
>> Do you prefer to use "static bool found_element" to mark if we already
>> have read an element size in
>>
>> the recursion function?
>>
>
> You could add a static, or you could use whether a set dsb or cmb size
> exists to mark that one was found, which I think is already partially 
> done.
>
> My confusion comes from the fact that instead of just a recursive
> search, the function doesn't stop at the first found one, it continues
> to sanity check that there is only one connected across all ports.
>
> And instead of just checking the error condition at the very end, it
> does it mid recursion, relying on the rc value from a previous
> iteration. IMO the following is a simplification because it always
> returns at the first error found, and checks the final condition outside
> of the recursive function. But you could probably leave it as you have
> but with some comments explaining why:
>
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c 
> b/drivers/hwtracing/coresight/coresight-tpda.c
> index 3101d2a0671d..00b7607d36be 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -90,13 +90,10 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                                  struct coresight_device *csdev,
>                                  int inport)
>  {
> -       int rc = -ENOENT;
> +       int rc = 0;
>         int i;
>         struct coresight_device *in;
>
> -       if (inport > 0)
> -               tpdm_clear_element_size(csdev);
> -
>         for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>                 in = csdev->pdata->in_conns[i]->src_dev;
>                 if (!in)
> @@ -108,19 +105,23 @@ static int tpda_get_element_size(struct 
> tpda_drvdata *drvdata,
>                         continue;
>
>                 if (coresight_device_is_tpdm(in)) {
> -                       if (rc)
> -                               rc = tpdm_read_element_size(drvdata, in);
> -                       else
> +                       if ((drvdata->dsb_esize) || 
> (drvdata->cmb_esize)) {
> +                               /* Error if already previously found 
> and initialised one */
> + dev_warn_once(&drvdata->csdev->dev,
> +                                       "Detected multiple TPDMs on 
> port %d", -EEXIST);
>                                 return -EINVAL;
> +                       }
> +                       rc = tpdm_read_element_size(drvdata, in);
> +                       if (rc)
> +                               return rc;
>                 } else {
>                         /* Recurse down the path */
>                         rc = tpda_get_element_size(drvdata, in, -1);
> +                       if (rc)
> +                               return rc;
>                 }
>         }
>
> -       if ((drvdata->dsb_esize) || (drvdata->cmb_esize))
> -               rc = 0;
> -
>         return rc;
>  }
>
> @@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata 
> *drvdata, int port)
>          * Set the bit to 0 if the size is 32
>          * Set the bit to 1 if the size is 64
>          */
> +       tpdm_clear_element_size(drvdata->csdev);
>         rc = tpda_get_element_size(drvdata, drvdata->csdev, port);
> -       if (!rc) {
> +       if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize)))
> +       {
>                 tpda_set_element_size(drvdata, &val);
>                 /* Enable the port */
>                 val |= TPDA_Pn_CR_ENA;
>                 writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> -       } else if (rc == -EINVAL) {
> -               dev_warn_once(&drvdata->csdev->dev,
> -                       "Detected multiple TPDMs on port %d", -EEXIST);
> +       } else {
> +               dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM 
> elem size");
> +               rc = -EINVAL;
>         }
>
>         return rc;

Make sense. I will refine and update the code in the next patch series.


Best,

Tao

>
>