Re: [PATCH v2 8/9] coresight: Enable and disable helper devices adjacent to the path

[James Clark <james.clark@arm.com>]

On 29/03/2023 14:23, Suzuki K Poulose wrote:
> On 29/03/2023 13:04, James Clark wrote:
>>
>>
>> On 17/03/2023 11:04, Suzuki K Poulose wrote:
>>> On 10/03/2023 16:06, James Clark wrote:
>>>> Currently CATU is the only helper device, and its enable and disable
>>>> calls are hard coded. To allow more helper devices to be added in a
>>>> generic way, remove these hard coded calls and just enable and disable
>>>> all helper devices.
>>>>
>>>> This has to apply to helpers adjacent to the path, because they will
>>>> never be in the path. CATU was already discovered in this way, so
>>>> there is no change there.
>>>>
>>>> One change that is needed is for CATU to call back into ETR to allocate
>>>> the buffer. Because the enable call was previously hard coded, it was
>>>> done at a point where the buffer was already allocated, but this is no
>>>> longer the case.
>>>>
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>> ---
>>>>    drivers/hwtracing/coresight/coresight-catu.c  | 34 ++++++++--
>>>>    drivers/hwtracing/coresight/coresight-core.c  | 68
>>>> ++++++++++++++++++-
>>>>    .../hwtracing/coresight/coresight-tmc-etr.c   | 28 --------
>>>>    include/linux/coresight.h                     |  3 +-
>>>>    4 files changed, 99 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-catu.c
>>>> b/drivers/hwtracing/coresight/coresight-catu.c
>>>> index bc90a03f478f..24a08a2b96b1 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-catu.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-catu.c
>>>> @@ -395,13 +395,32 @@ static inline int catu_wait_for_ready(struct
>>>> catu_drvdata *drvdata)
>>>>        return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY,
>>>> 1);
>>>>    }
>>>>    -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
>>>> +static struct coresight_device *
>>>> +catu_get_etr_device(struct coresight_device *csdev)
>>>> +{
>>>> +    int i;
>>>> +    struct coresight_device *tmp;
>>>> +
>>>> +    for (i = 0; i < csdev->pdata->nr_inconns; i++) {
>>>> +        tmp = csdev->pdata->in_conns[i].remote_dev;
>>>> +        if (tmp && tmp->type == CORESIGHT_DEV_TYPE_SINK &&
>>>> +            tmp->subtype.sink_subtype ==
>>>> +                CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM)
>>>> +            return tmp;
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode
>>>> cs_mode,
>>>> +              void *data)
>>>>    {
>>>>        int rc;
>>>>        u32 control, mode;
>>>> -    struct etr_buf *etr_buf = data;
>>>> +    struct etr_buf *etr_buf = NULL;
>>>>        struct device *dev = &drvdata->csdev->dev;
>>>>        struct coresight_device *csdev = drvdata->csdev;
>>>> +    struct coresight_device *etrdev;
>>>>          if (catu_wait_for_ready(drvdata))
>>>>            dev_warn(dev, "Timeout while waiting for READY\n");
>>>> @@ -416,6 +435,12 @@ static int catu_enable_hw(struct catu_drvdata
>>>> *drvdata, void *data)
>>>>        if (rc)
>>>>            return rc;
>>>>    +    etrdev = catu_get_etr_device(csdev);
>>>> +    if (etrdev) {
>>>> +        etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
>>>> +        if (IS_ERR(etr_buf))
>>>> +            return PTR_ERR(etr_buf);
>>>> +    }
>>>
>>> WARN_ON(!etrdev) ? We are not supposed to reach in the first place and
>>> return.
>>>
>>
>> I saw there was the pass-through mode below which I thought didn't need
>> an ETR device. I think I followed the code through and there was a way
>> for it to get there without an ETR in the existing version, but now I'm
>> not sure.
> 
> 
>> Or does it still need the ETR device but it just doesn't
>> access the buffer?
> 
> The first part is correct. Without an ETR, CATU wouldn't be a helper
> device, and wouldn't get here in "enable CATU" via the helper route.
> The CATU chooses the mode depending on the etr_buf mode.

Ok thanks I will add that warning then

> 
> 
> Suzuki
>