Messages in this thread Patch in this message | | | Date | Thu, 27 Jan 2022 17:33:26 +0800 | Subject | Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver | From | Jinlong Mao <> |
| |
On 1/26/2022 11:34 PM, Mathieu Poirier wrote: > On Wed, Jan 26, 2022 at 03:07:38PM +0800, Jinlong Mao wrote: >> 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 ? > I just received clarifications on this - there is no need to continue enacting > the current scheme and as such reserving bits following the above formula will > not be needed either. Simply assigning free traceIDs based on request will > work just fine. > > That of course is taking into account that 0x0, 0x1 and 0x70 - 0x7f are > reserved. 0x1 is currently used for STM and should eventually be fixed to > simply request a traceID the same way any other component do. You can fix it as > part of this set but it is not mandatory. > > Let me know if there are things I haven't been clear on. > > Thanks, > Mathieu
Hi Mathieu,
Do you mean ETM sources and other sources can share the same "coresight_get_trace_id" function ?
int trace_id = 1;
int coresight_get_trace_id () { trace_id += 1; if (trace_id > 1 && trace_id < 0x70) return trace_id; else return -EINVAL; }
or do we still need a different function for other sources like use "coresight_get_system_trace_id" ? Return the id from 1 and skip the ids which equeals to CORESIGHT_ETM_PMU_SEED + (cpu * 2).
Thanks
Jinlong Mao
>>>>> 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
| |