Messages in this thread | | | Date | Wed, 26 Jan 2022 15:07:38 +0800 | Subject | Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver | From | Jinlong Mao <> |
| |
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
| |