lkml.org 
[lkml]   [2022]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver
From
Hi Mathieu,

Good afternoon.

On 1/22/2022 1:15 AM, Mathieu Poirier wrote:
> Hey Jinlong,
>
> On Fri, Jan 21, 2022 at 10:01:47PM +0800, Jinlong Mao wrote:
>> 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)?
> Yes, you are correct.
>
>>> 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.
> I need to clarify something with Mike on this - I will get back to you on
> Monday.
Do you have more comments on this ?
>
>>> 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

\
 
 \ /
  Last update: 2022-01-26 08:08    [W:0.150 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site