From: Jinlong Mao <quic_jinlmao@quicinc.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Leo Yan <leo.yan@linaro.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
<coresight@lists.linaro.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>,
Tingwei Zhang <quic_tingweiz@quicinc.com>,
Yuanfang Zhang <quic_yuanfang@quicinc.com>,
Tao Zhang <quic_taozha@quicinc.com>,
Trilok Soni <quic_tsoni@quicinc.com>,
<linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver
Date: Thu, 27 Jan 2022 17:33:26 +0800 [thread overview]
Message-ID: <25d9a681-b899-5936-868d-34f645ee40ef@quicinc.com> (raw)
In-Reply-To: <20220126153437.GA2144852@p14s>
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
next prev parent reply other threads:[~2022-01-27 9:33 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-09 14:15 [PATCH v2 0/9] Coresight: Add support for TPDM and TPDA Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 1/9] Use IDR to maintain all the enabled sources' paths Mao Jinlong
2021-12-14 18:30 ` Mathieu Poirier
2021-12-15 15:40 ` Jinlong Mao
2021-12-09 14:15 ` [PATCH v2 2/9] Coresight: Add coresight TPDM source driver Mao Jinlong
2021-12-14 18:57 ` Mathieu Poirier
2021-12-15 16:10 ` Jinlong Mao
2021-12-16 17:45 ` Mike Leach
2021-12-16 19:02 ` Mathieu Poirier
2022-01-21 14:01 ` Jinlong Mao
2022-01-21 17:15 ` Mathieu Poirier
2022-01-26 7:07 ` Jinlong Mao
2022-01-26 15:34 ` Mathieu Poirier
2022-01-27 9:33 ` Jinlong Mao [this message]
2022-01-27 18:15 ` Mathieu Poirier
2021-12-09 14:15 ` [PATCH v2 3/9] dt-bindings: arm: Adds CoreSight TPDM hardware definitions Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 4/9] coresight-tpdm: Add DSB dataset support Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 5/9] coresight-tpdm: Add integration test support Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 6/9] docs: sysfs: coresight: Add sysfs ABI documentation for TPDM Mao Jinlong
2021-12-09 14:25 ` Jinlong Mao
2021-12-09 18:34 ` Trilok Soni
2021-12-10 1:03 ` Jinlong Mao
2021-12-17 16:09 ` Greg Kroah-Hartman
2021-12-09 14:15 ` [PATCH v2 7/9] Coresight: Add TPDA link driver Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 8/9] dt-bindings: arm: Adds CoreSight TPDA hardware definitions Mao Jinlong
2021-12-09 14:15 ` [PATCH v2 9/9] ARM: dts: msm: Add coresight components for SM8250 Mao Jinlong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=25d9a681-b899-5936-868d-34f645ee40ef@quicinc.com \
--to=quic_jinlmao@quicinc.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=coresight@lists.linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=quic_taozha@quicinc.com \
--cc=quic_tingweiz@quicinc.com \
--cc=quic_tsoni@quicinc.com \
--cc=quic_yuanfang@quicinc.com \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).