Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver

[Jinlong Mao <quic_jinlmao@quicinc.com>]

Hi Mathieu,

Good Day.

On 12/17/2021 3:02 AM, Mathieu Poirier wrote:
> [...]
>
>>>>> +
>>>>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>>>>> +{
>>>>> +    static int traceid = TPDM_TRACE_ID_START;
>>>>> +
>>>>> +    drvdata->traceid = traceid++;
>>>>> +}
>>>> I have been specific on how to properly do this in the last revision.  Given the
>>>> above about the MAINTAINERS file, I am not sure that I will continue reviewing this set.
>>>>
>>>> There is also no need to rush another revision as I won't have the bandwidth to
>>>> process it before the holidays.
>>>>
>>>> Thanks,
>>>> Mathieu
>>> Hi Mathieu,
>>>
>>> Sorry, not addressed your previous comments here.
>>>
>>> For the trace id, each coresight component has 7 bits to store the trace
>>> id. So the trace id should be from 1 to 127 as 0 is invalid.
>> IDs 0x70 - 0x7F (`112 - 127 ) are reserved - see the ARM Coresight
>> Architecture specification v3.0
>>
> Correct
>
>>> Apart from TPDMs/STM/ETMs, we also have other coresight components in
>>> our internal device. About 80 ids are already used.
>>>
>>> Some components have fixed trace id in HW. If we use functions below to
>>> count the trace id, there will be conflict to other components.
>>>
>>> Can we use 1-15 for etm trace ids  and 16 - 127 for other coresight
>>> components ? And handle trace ids in its' own driver ?
>>>
>> This will limit systems to 15 cores - some have more!
>>
> Correct
>
>>> static inline int coresight_get_system_trace_id(int id)
>>> {
>>>           /* Start system IDs above the highest per CPU trace ID. */
>>>           return coresigth_get_trace_id(cpumask_last(cpu_possible_mask) + 1);
>>> }
> Looking at my own suggestion again this won't work since it returns the same traceID
> when called multiple times.
>
> For this patchset and _without_ taking into account internal devices that have
> their traceID set in HW:
>
> 1. Define a bitmask that is 7 bit wide.
Should it be a 128 bit wide bitmask  (0--127)?
> 2. By default, set bits under 0x10 and between 0x70 - 0x7F.
> 3. In coresight_get_system_trace_id(), drop the @id parameter and allocate the
> first available bit after cpumask_last(cpu_possible_mask) + 1.

Should it allocate the first available bit after (cpumask_last(cpu_possible_mask) *2 ) + 0x10 ?
Return the first zero bit position as the trace id and set the bit.

> 4. Define a new function called coresight_put_system_trace_id(int id) that
> clears the bit in the mask corresponding to @id.
>
> For now that should work.
>
>>> static inline int coresight_get_trace_id(int cpu)
>>> {
>>>       /*
>>>        * A trace ID of value 0 is invalid, so let's start at some
>>>        * random value that fits in 7 bits and go from there.  Since
>>>        * the common convention is to have data trace IDs be I(N) + 1,
>>>        * set instruction trace IDs as a function of the CPU number.
>>>        */
>>>       return (CORESIGHT_ETM_PMU_SEED + (cpu * 2));
>>> }
>>>
>> This fixed relationship between cpu and trace ID is used in the perf
>> tooling to populate the elements in the perf.data file to correctly
>> allow association between CPU and trace data, and thus allow correct
>> trace decode.
> TraceIDs associated to CPUs are communicated to the perf tooling by way of the
> perf header - theoretically we should be able to change the allocation scheme
> without impacting the decoding process.
>
>> It should be possible to create another more dynamic mapping scheme -
>> but this must include a way to support the perf requirements too.
>>
> TraceIDs have been a lurking problem for as long as the subsystem has existed.
> For now what I have suggested above should be sufficient to provide an
> in-between solution that doesn't hold back this patchset.
>
> That being said, we need to start thinking about the best way to do this.  I
> will put a patchset together in the new year that aims in that direction.
>
>> Regards
>>
>> Mike
>>
>>> Thanks
>>>
>>> Jinlong Mao
>>>
>>>>> +
>>>>> +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>>>>> +{
>>>>> +    struct device *dev = &adev->dev;
>>>>> +    struct coresight_platform_data *pdata;
>>>>> +    struct tpdm_drvdata *drvdata;
>>>>> +    struct coresight_desc desc = { 0 };
>>>>> +
>>>>> +    desc.name = coresight_alloc_device_name(&tpdm_devs, dev);
>>>>> +    if (!desc.name)
>>>>> +            return -ENOMEM;
>>>>> +    pdata = coresight_get_platform_data(dev);
>>>>> +    if (IS_ERR(pdata))
>>>>> +            return PTR_ERR(pdata);
>>>>> +    adev->dev.platform_data = pdata;
>>>>> +
>>>>> +    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>>>>> +    if (!drvdata)
>>>>> +            return -ENOMEM;
>>>>> +    drvdata->dev = &adev->dev;
>>>>> +    dev_set_drvdata(dev, drvdata);
>>>>> +
>>>>> +    drvdata->base = devm_ioremap_resource(dev, &adev->res);
>>>>> +    if (!drvdata->base)
>>>>> +            return -ENOMEM;
>>>>> +
>>>>> +    mutex_init(&drvdata->lock);
>>>>> +
>>>>> +    desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>>>> +    desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SYS;
>>>>> +    desc.ops = &tpdm_cs_ops;
>>>>> +    desc.pdata = adev->dev.platform_data;
>>>>> +    desc.dev = &adev->dev;
>>>>> +    drvdata->csdev = coresight_register(&desc);
>>>>> +    if (IS_ERR(drvdata->csdev))
>>>>> +            return PTR_ERR(drvdata->csdev);
>>>>> +
>>>>> +    tpdm_init_default_data(drvdata);
>>>>> +    pm_runtime_put(&adev->dev);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void __exit tpdm_remove(struct amba_device *adev)
>>>>> +{
>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>>>>> +
>>>>> +    coresight_unregister(drvdata->csdev);
>>>>> +}
>>>>> +
>>>>> +static struct amba_id tpdm_ids[] = {
>>>>> +    {
>>>>> +            .id = 0x000f0e00,
>>>>> +            .mask = 0x000fff00,
>>>>> +    },
>>>>> +    { 0, 0},
>>>>> +};
>>>>> +
>>>>> +static struct amba_driver tpdm_driver = {
>>>>> +    .drv = {
>>>>> +            .name   = "coresight-tpdm",
>>>>> +            .owner  = THIS_MODULE,
>>>>> +            .suppress_bind_attrs = true,
>>>>> +    },
>>>>> +    .probe          = tpdm_probe,
>>>>> +    .id_table       = tpdm_ids,
>>>>> +};
>>>>> +
>>>>> +module_amba_driver(tpdm_driver);
>>>>> +
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver");
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> new file mode 100644
>>>>> index 000000000000..980ae90ff1c8
>>>>> --- /dev/null
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>>> @@ -0,0 +1,31 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>> + */
>>>>> +
>>>>> +#ifndef _CORESIGHT_CORESIGHT_TPDM_H
>>>>> +#define _CORESIGHT_CORESIGHT_TPDM_H
>>>>> +
>>>>> +/* Default value of the traceid */
>>>>> +#define TPDM_TRACE_ID_START 128
>>>>> +
>>>>> +/**
>>>>> + * struct tpdm_drvdata - specifics associated to an TPDM component
>>>>> + * @base:       memory mapped base address for this component.
>>>>> + * @dev:        The device entity associated to this component.
>>>>> + * @csdev:      component vitals needed by the framework.
>>>>> + * @lock:       lock for the enable value.
>>>>> + * @enable:     enable status of the component.
>>>>> + * @traceid:    value of the current ID for this component.
>>>>> + */
>>>>> +
>>>>> +struct tpdm_drvdata {
>>>>> +    void __iomem            *base;
>>>>> +    struct device           *dev;
>>>>> +    struct coresight_device *csdev;
>>>>> +    struct mutex            lock;
>>>>> +    bool                    enable;
>>>>> +    int                     traceid;
>>>>> +};
>>>>> +
>>>>> +#endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
>>>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>>>> index 93a2922b7653..e48d463be63b 100644
>>>>> --- a/include/linux/coresight.h
>>>>> +++ b/include/linux/coresight.h
>>>>> @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source {
>>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
>>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>>>>> +    CORESIGHT_DEV_SUBTYPE_SOURCE_SYS,
>>>>>    };
>>>>>
>>>>>    enum coresight_dev_subtype_helper {
>>>>> --
>>>>> 2.17.1
>>>>>
>>
>>
>> -- 
>> Mike Leach
>> Principal Engineer, ARM Ltd.
>> Manchester Design Centre. UK